From dbc554cda7f86f22d34fd1e3a4ec4ab677d55879 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 1 Sep 2021 06:09:00 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/graphql/types/project_statistics_type.rb | 2 + app/models/concerns/loose_foreign_key.rb | 95 +++++++++++++++++++ app/models/environment.rb | 41 +------- .../environments/auto_stop_service.rb | 12 ++- app/services/environments/stop_service.rb | 16 ---- app/workers/all_queues.yml | 9 ++ app/workers/environments/auto_stop_worker.rb | 18 ++++ config/sidekiq_queues.yml | 2 + doc/api/graphql/reference/index.md | 1 + doc/development/code_review.md | 2 +- .../documentation/styleguide/word_list.md | 20 ++-- doc/topics/git/index.md | 4 +- .../index.md | 2 +- .../merge_requests/cherry_pick_changes.md | 6 +- .../project/merge_requests/revert_changes.md | 6 +- .../merge_requests/squash_and_merge.md | 6 +- doc/user/project/repository/index.md | 4 +- .../types/project_statistics_type_spec.rb | 2 +- .../models/concerns/loose_foreign_key_spec.rb | 83 ++++++++++++++++ spec/models/environment_spec.rb | 63 +++--------- .../environments/auto_stop_service_spec.rb | 11 ++- .../environments/stop_service_spec.rb | 54 ----------- .../environments/auto_stop_worker_spec.rb | 71 ++++++++++++++ 23 files changed, 344 insertions(+), 186 deletions(-) create mode 100644 app/models/concerns/loose_foreign_key.rb create mode 100644 app/workers/environments/auto_stop_worker.rb create mode 100644 spec/models/concerns/loose_foreign_key_spec.rb create mode 100644 spec/workers/environments/auto_stop_worker_spec.rb diff --git a/app/graphql/types/project_statistics_type.rb b/app/graphql/types/project_statistics_type.rb index ee8476f50de..60a3d5ce06b 100644 --- a/app/graphql/types/project_statistics_type.rb +++ b/app/graphql/types/project_statistics_type.rb @@ -23,6 +23,8 @@ module Types description: 'Wiki size of the project in bytes.' field :snippets_size, GraphQL::FLOAT_TYPE, null: true, description: 'Snippets size of the project in bytes.' + field :pipeline_artifacts_size, GraphQL::FLOAT_TYPE, null: true, + description: 'CI Pipeline artifacts size in bytes.' field :uploads_size, GraphQL::FLOAT_TYPE, null: true, description: 'Uploads size of the project in bytes.' end diff --git a/app/models/concerns/loose_foreign_key.rb b/app/models/concerns/loose_foreign_key.rb new file mode 100644 index 00000000000..4e822a04869 --- /dev/null +++ b/app/models/concerns/loose_foreign_key.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +module LooseForeignKey + extend ActiveSupport::Concern + + # This concern adds loose foreign key support to ActiveRecord models. + # Loose foreign keys allow delayed processing of associated database records + # with similar guarantees than a database foreign key. + # + # TODO: finalize this later once the async job is in place + # + # Prerequisites: + # + # To start using the concern, you'll need to install a database trigger to the parent + # table in a standard DB migration (not post-migration). + # + # > add_loose_foreign_key_support(:projects, :gitlab_main) + # + # Usage: + # + # > class Ci::Build < ApplicationRecord + # > + # > loose_foreign_key :security_scans, :build_id, on_delete: :async_delete, gitlab_schema: :gitlab_main + # > + # > # associations can be still defined, the dependent options is no longer necessary: + # > has_many :security_scans, class_name: 'Security::Scan' + # > + # > end + # + # Options for on_delete: + # + # - :async_delete - deletes the children rows via an asynchronous process. + # - :async_nullify - sets the foreign key column to null via an asynchronous process. + # + # Options for gitlab_schema: + # + # - :gitlab_ci + # - :gitlab_main + # + # The value can be determined by calling `Model.gitlab_schema` where the Model represents + # the model for the child table. + # + # How it works: + # + # When adding loose foreign key support to the table, a DELETE trigger is installed + # which tracks the record deletions (stores primary key value of the deleted row) in + # a database table. + # + # These deletion records are processed asynchronously and records are cleaned up + # according to the loose foreign key definitions described in the model. + # + # The cleanup happens in batches, which reduces the likelyhood of statement timeouts. + # + # When all associations related to the deleted record are cleaned up, the record itself + # is deleted. + included do + class_attribute :loose_foreign_key_definitions, default: [] + end + + class_methods do + def loose_foreign_key(to_table, column, options) + symbolized_options = options.symbolize_keys + + unless base_class? + raise <<~MSG + loose_foreign_key can be only used on base classes, inherited classes are not supported. + Please define the loose_foreign_key on the #{base_class.name} class. + MSG + end + + on_delete_options = %i[async_delete async_nullify] + gitlab_schema_options = [ApplicationRecord.gitlab_schema, Ci::ApplicationRecord.gitlab_schema] + + unless on_delete_options.include?(symbolized_options[:on_delete]&.to_sym) + raise "Invalid on_delete option given: #{symbolized_options[:on_delete]}. Valid options: #{on_delete_options.join(', ')}" + end + + unless gitlab_schema_options.include?(symbolized_options[:gitlab_schema]&.to_sym) + raise "Invalid gitlab_schema option given: #{symbolized_options[:gitlab_schema]}. Valid options: #{gitlab_schema_options.join(', ')}" + end + + definition = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + table_name.to_s, + to_table.to_s, + { + column: column.to_s, + on_delete: symbolized_options[:on_delete].to_sym, + gitlab_schema: symbolized_options[:gitlab_schema].to_sym + } + ) + + self.loose_foreign_key_definitions += [definition] + end + end +end diff --git a/app/models/environment.rb b/app/models/environment.rb index 963249c018a..a4084565579 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -77,6 +77,7 @@ class Environment < ApplicationRecord scope :in_review_folder, -> { where(environment_type: "review") } scope :for_name, -> (name) { where(name: name) } scope :preload_cluster, -> { preload(last_deployment: :cluster) } + scope :preload_project, -> { preload(:project) } scope :auto_stoppable, -> (limit) { available.where('auto_stop_at < ?', Time.zone.now).limit(limit) } scope :auto_deletable, -> (limit) { stopped.where('auto_delete_at < ?', Time.zone.now).limit(limit) } @@ -132,6 +133,10 @@ class Environment < ApplicationRecord state :available state :stopped + before_transition any => :stopped do |environment| + environment.auto_stop_at = nil + end + after_transition do |environment| environment.expire_etag_cache end @@ -168,33 +173,6 @@ class Environment < ApplicationRecord end class << self - ## - # This method returns stop actions (jobs) for multiple environments within one - # query. It's useful to avoid N+1 problem. - # - # NOTE: The count of environments should be small~medium (e.g. < 5000) - def stop_actions - cte = cte_for_deployments_with_stop_action - ci_builds = Ci::Build.arel_table - - inner_join_stop_actions = ci_builds.join(cte.table).on( - ci_builds[:project_id].eq(cte.table[:project_id]) - .and(ci_builds[:ref].eq(cte.table[:ref])) - .and(ci_builds[:name].eq(cte.table[:on_stop])) - ).join_sources - - pipeline_ids = ci_builds.join(cte.table).on( - ci_builds[:id].eq(cte.table[:deployable_id]) - ).project(:commit_id) - - Ci::Build.joins(inner_join_stop_actions) - .with(cte.to_arel) - .where(ci_builds[:commit_id].in(pipeline_ids)) - .where(status: Ci::HasStatus::BLOCKED_STATUS) - .preload_project_and_pipeline_project - .preload(:user, :metadata, :deployment) - end - def count_by_state environments_count_by_state = group(:state).count @@ -202,15 +180,6 @@ class Environment < ApplicationRecord count_hash[state] = environments_count_by_state[state.to_s] || 0 end end - - private - - def cte_for_deployments_with_stop_action - Gitlab::SQL::CTE.new(:deployments_with_stop_action, - Deployment.where(environment_id: select(:id)) - .distinct_on_environment - .stoppable) - end end def clear_prometheus_reactive_cache!(query_name) diff --git a/app/services/environments/auto_stop_service.rb b/app/services/environments/auto_stop_service.rb index 4e3aec64283..686ba050326 100644 --- a/app/services/environments/auto_stop_service.rb +++ b/app/services/environments/auto_stop_service.rb @@ -28,11 +28,17 @@ module Environments private def stop_in_batch - environments = Environment.auto_stoppable(BATCH_SIZE) + environments = Environment.preload_project.select(:id, :project_id).auto_stoppable(BATCH_SIZE) - return false unless environments.exists? + return false if environments.empty? - Environments::StopService.execute_in_batch(environments) + Environments::AutoStopWorker.bulk_perform_async_with_contexts( + environments, + arguments_proc: -> (environment) { environment.id }, + context_proc: -> (environment) { { project: environment.project } } + ) + + true end end end diff --git a/app/services/environments/stop_service.rb b/app/services/environments/stop_service.rb index 089aea11296..d9c66bd13fe 100644 --- a/app/services/environments/stop_service.rb +++ b/app/services/environments/stop_service.rb @@ -22,22 +22,6 @@ module Environments merge_request.environments.each { |environment| execute(environment) } end - ## - # This method is for stopping multiple environments in a batch style. - # The maximum acceptable count of environments is roughly 5000. Please - # apply acceptable `LIMIT` clause to the `environments` relation. - def self.execute_in_batch(environments) - stop_actions = environments.stop_actions.load - - environments.update_all(auto_stop_at: nil, state: 'stopped') - - stop_actions.each do |stop_action| - stop_action.play(stop_action.user) - rescue StandardError => e - Gitlab::ErrorTracking.track_exception(e, deployable_id: stop_action.id) - end - end - private def environments diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 5218abb67c9..d4588ee78a4 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -1997,6 +1997,15 @@ :weight: 2 :idempotent: :tags: [] +- :name: environments_auto_stop + :worker_name: Environments::AutoStopWorker + :feature_category: :continuous_delivery + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: environments_canary_ingress_update :worker_name: Environments::CanaryIngress::UpdateWorker :feature_category: :continuous_delivery diff --git a/app/workers/environments/auto_stop_worker.rb b/app/workers/environments/auto_stop_worker.rb new file mode 100644 index 00000000000..672a4f4121e --- /dev/null +++ b/app/workers/environments/auto_stop_worker.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Environments + class AutoStopWorker + include ApplicationWorker + + data_consistency :always + idempotent! + feature_category :continuous_delivery + + def perform(environment_id, params = {}) + Environment.find_by_id(environment_id).try do |environment| + user = environment.stop_action&.user + environment.stop_with_action!(user) + end + end + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 52ecd62edd5..2c2031ce2ae 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -126,6 +126,8 @@ - 2 - - emails_on_push - 2 +- - environments_auto_stop + - 1 - - environments_canary_ingress_update - 1 - - epics diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index e477c2fecf9..d83b4da98ba 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -13021,6 +13021,7 @@ Represents a Project Membership. | `commitCount` | [`Float!`](#float) | Commit count of the project. | | `lfsObjectsSize` | [`Float!`](#float) | Large File Storage (LFS) object size of the project in bytes. | | `packagesSize` | [`Float!`](#float) | Packages size of the project in bytes. | +| `pipelineArtifactsSize` | [`Float`](#float) | CI Pipeline artifacts size in bytes. | | `repositorySize` | [`Float!`](#float) | Repository size of the project in bytes. | | `snippetsSize` | [`Float`](#float) | Snippets size of the project in bytes. | | `storageSize` | [`Float!`](#float) | Storage size of the project in bytes. | diff --git a/doc/development/code_review.md b/doc/development/code_review.md index d66f246ac8c..f7046216d3d 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -438,7 +438,7 @@ WARNING: - For merge requests that have had [Squash and merge](../user/project/merge_requests/squash_and_merge.md#squash-and-merge) set, the squashed commit’s default commit message is taken from the merge request title. - You're encouraged to [select a commit with a more informative commit message](../user/project/merge_requests/squash_and_merge.md#overview) before merging. + You're encouraged to [select a commit with a more informative commit message](../user/project/merge_requests/squash_and_merge.md) before merging. Thanks to **Pipeline for Merged Results**, authors no longer have to rebase their branch as frequently anymore (only when there are conflicts) because the Merge diff --git a/doc/development/documentation/styleguide/word_list.md b/doc/development/documentation/styleguide/word_list.md index 228a0467b88..ca8aa284241 100644 --- a/doc/development/documentation/styleguide/word_list.md +++ b/doc/development/documentation/styleguide/word_list.md @@ -50,7 +50,7 @@ in the handbook when writing about Alpha features. ## and/or -Instead of **and/or**, use or or rewrite the sentence to spell out both options. +Instead of **and/or**, use **or** or rewrite the sentence to spell out both options. ## area @@ -130,8 +130,8 @@ When writing about the Developer role: - Do not use the phrase, **if you are a developer** to mean someone who is assigned the Developer role. Instead, write it out. For example, **if you are assigned the Developer role**. - To describe a situation where the Developer role is the minimum required: - - Avoid: **the Developer role or higher** - - Use instead: **at least the Developer role** + - Avoid: the Developer role or higher + - Use instead: at least the Developer role Do not use **Developer permissions**. A user who is assigned the Developer role has a set of associated permissions. @@ -205,7 +205,7 @@ Do not use in product documentation. You can use it in our API and contributor d ## future tense -When possible, use present tense instead. For example, use `after you execute this command, GitLab displays the result` instead of `after you execute this command, GitLab will display the result`. ([Vale](../testing.md#vale) rule: [`FutureTense.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/.vale/gitlab/FutureTense.yml)) +When possible, use present tense instead. For example, use **after you execute this command, GitLab displays the result** instead of **after you execute this command, GitLab will display the result**. ([Vale](../testing.md#vale) rule: [`FutureTense.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/.vale/gitlab/FutureTense.yml)) ## Geo @@ -241,8 +241,8 @@ When writing about the Guest role: - Do not use the phrase, **if you are a guest** to mean someone who is assigned the Guest role. Instead, write it out. For example, **if you are assigned the Guest role**. - To describe a situation where the Guest role is the minimum required: - - Avoid: **the Guest role or higher** - - Use instead: **at least the Guest role** + - Avoid: the Guest role or higher + - Use instead: at least the Guest role Do not use **Guest permissions**. A user who is assigned the Guest role has a set of associated permissions. @@ -333,8 +333,8 @@ When writing about the Maintainer role: - Do not use the phrase, **if you are a maintainer** to mean someone who is assigned the Maintainer role. Instead, write it out. For example, **if you are assigned the Maintainer role**. - To describe a situation where the Maintainer role is the minimum required: - - Avoid: **the Maintainer role or higher** - - Use instead: **at least the Maintainer role** + - Avoid: the Maintainer role or higher + - Use instead: at least the Maintainer role Do not use **Maintainer permissions**. A user who is assigned the Maintainer role has a set of associated permissions. @@ -433,8 +433,8 @@ When writing about the Reporter role: - Do not use the phrase, **if you are a reporter** to mean someone who is assigned the Reporter role. Instead, write it out. For example, **if you are assigned the Reporter role**. - To describe a situation where the Reporter role is the minimum required: - - Avoid: **the Reporter role or higher** - - Use instead: **at least the Reporter role** + - Avoid: the Reporter role or higher + - Use instead: at least the Reporter role Do not use **Reporter permissions**. A user who is assigned the Reporter role has a set of associated permissions. diff --git a/doc/topics/git/index.md b/doc/topics/git/index.md index c1e766a7c48..cdbe4dc7623 100644 --- a/doc/topics/git/index.md +++ b/doc/topics/git/index.md @@ -34,8 +34,8 @@ The following resources can help you get started with Git: - [Edit files through the command line](../../gitlab-basics/command-line-commands.md) - [GitLab Git Cheat Sheet (download)](https://about.gitlab.com/images/press/git-cheat-sheet.pdf) - Commits: - - [Revert a commit](../../user/project/merge_requests/revert_changes.md#reverting-a-commit) - - [Cherry-picking a commit](../../user/project/merge_requests/cherry_pick_changes.md#cherry-picking-a-commit) + - [Revert a commit](../../user/project/merge_requests/revert_changes.md#revert-a-commit) + - [Cherry-picking a commit](../../user/project/merge_requests/cherry_pick_changes.md#cherry-pick-a-commit) - [Squashing commits](../gitlab_flow.md#squashing-commits-with-rebase) - [Squash-and-merge](../../user/project/merge_requests/squash_and_merge.md) - [Signing commits](../../user/project/repository/gpg_signed_commits/index.md) diff --git a/doc/topics/git/numerous_undo_possibilities_in_git/index.md b/doc/topics/git/numerous_undo_possibilities_in_git/index.md index 4d58c7ab455..0aecb48e497 100644 --- a/doc/topics/git/numerous_undo_possibilities_in_git/index.md +++ b/doc/topics/git/numerous_undo_possibilities_in_git/index.md @@ -209,7 +209,7 @@ To recover from multiple incorrect commits: The commits are now `A-B-C-D-E`. Alternatively, with GitLab, -you can [cherry-pick](../../../user/project/merge_requests/cherry_pick_changes.md#cherry-picking-a-commit) +you can [cherry-pick](../../../user/project/merge_requests/cherry_pick_changes.md#cherry-pick-a-commit) that commit into a new merge request. NOTE: diff --git a/doc/user/project/merge_requests/cherry_pick_changes.md b/doc/user/project/merge_requests/cherry_pick_changes.md index a0c3efe7143..4a2319774ac 100644 --- a/doc/user/project/merge_requests/cherry_pick_changes.md +++ b/doc/user/project/merge_requests/cherry_pick_changes.md @@ -11,7 +11,7 @@ GitLab implements Git's powerful feature to [cherry-pick any commit](https://git-scm.com/docs/git-cherry-pick "Git cherry-pick documentation") with a **Cherry-pick** button in merge requests and commit details. -## Cherry-picking a merge request +## Cherry-pick a merge request After the merge request has been merged, a **Cherry-pick** button displays to cherry-pick the changes introduced by that merge request. @@ -25,7 +25,7 @@ where you can choose to either: - Cherry-pick the changes directly into the selected branch. - Create a new merge request with the cherry-picked changes. -### Cherry-pick tracking +### Track a cherry-pick > [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/2675) in GitLab 12.9. @@ -40,7 +40,7 @@ NOTE: We only track cherry-pick executed from GitLab (both UI and API). Support for tracking cherry-picked commits through the command line is tracked [in this issue](https://gitlab.com/gitlab-org/gitlab/-/issues/202215). -## Cherry-picking a commit +## Cherry-pick a commit You can cherry-pick a commit from the commit details page: diff --git a/doc/user/project/merge_requests/revert_changes.md b/doc/user/project/merge_requests/revert_changes.md index a798f2c9b85..e6fb619d365 100644 --- a/doc/user/project/merge_requests/revert_changes.md +++ b/doc/user/project/merge_requests/revert_changes.md @@ -5,12 +5,12 @@ info: To determine the technical writer assigned to the Stage/Group associated w type: reference, concepts --- -# Reverting changes **(FREE)** +# Revert changes **(FREE)** You can use Git's powerful feature to [revert any commit](https://git-scm.com/docs/git-revert "Git revert documentation") by clicking the **Revert** button in merge requests and commit details. -## Reverting a merge request +## Revert a merge request NOTE: The **Revert** button is available only for merge requests @@ -34,7 +34,7 @@ create a new merge request with the revert changes. After the merge request has been reverted, the **Revert** button is no longer available. -## Reverting a commit +## Revert a commit You can revert a commit from the commit details page: diff --git a/doc/user/project/merge_requests/squash_and_merge.md b/doc/user/project/merge_requests/squash_and_merge.md index 2842c084bc5..c3fc2fa871f 100644 --- a/doc/user/project/merge_requests/squash_and_merge.md +++ b/doc/user/project/merge_requests/squash_and_merge.md @@ -12,8 +12,6 @@ type: reference, concepts With squash and merge you can combine all your merge request's commits into one and retain a clean history. -## Overview - Squashing lets you tidy up the commit history of a branch when accepting a merge request. It applies all of the changes in the merge request as a single commit, and then merges that commit using the merge method set for the project. @@ -58,7 +56,7 @@ meaningful commit messages and: - It's simpler to [revert](revert_changes.md) if necessary. - The merged branch retains the full commit history. -## Enabling squash for a merge request +## Enable squash for a merge request Anyone who can create or edit a merge request can choose for it to be squashed on the merge request form. Users can select or clear the checkbox when they @@ -98,7 +96,7 @@ it. This is because squashing is only available when accepting a merge request, so a merge request may need to be rebased before squashing, even though squashing can itself be considered equivalent to rebasing. -## Squash Commits Options +## Squash commits options > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/17613) in GitLab 13.2. > - Deployed behind a feature flag, disabled by default. diff --git a/doc/user/project/repository/index.md b/doc/user/project/repository/index.md index afdcf2a94fa..26b9a979c34 100644 --- a/doc/user/project/repository/index.md +++ b/doc/user/project/repository/index.md @@ -50,10 +50,10 @@ to a branch in the repository. When you use the command line, you can commit mul on their respective thread. - **Cherry-pick a commit:** In GitLab, you can - [cherry-pick a commit](../merge_requests/cherry_pick_changes.md#cherry-picking-a-commit) + [cherry-pick a commit](../merge_requests/cherry_pick_changes.md#cherry-pick-a-commit) from the UI. - **Revert a commit:** - [Revert a commit](../merge_requests/revert_changes.md#reverting-a-commit) + [Revert a commit](../merge_requests/revert_changes.md#revert-a-commit) from the UI to a selected branch. - **Sign a commit:** Use GPG to [sign your commits](gpg_signed_commits/index.md). diff --git a/spec/graphql/types/project_statistics_type_spec.rb b/spec/graphql/types/project_statistics_type_spec.rb index 407ce82e73a..f515907b6a8 100644 --- a/spec/graphql/types/project_statistics_type_spec.rb +++ b/spec/graphql/types/project_statistics_type_spec.rb @@ -6,6 +6,6 @@ RSpec.describe GitlabSchema.types['ProjectStatistics'] do it 'has all the required fields' do expect(described_class).to have_graphql_fields(:storage_size, :repository_size, :lfs_objects_size, :build_artifacts_size, :packages_size, :commit_count, - :wiki_size, :snippets_size, :uploads_size) + :wiki_size, :snippets_size, :pipeline_artifacts_size, :uploads_size) end end diff --git a/spec/models/concerns/loose_foreign_key_spec.rb b/spec/models/concerns/loose_foreign_key_spec.rb new file mode 100644 index 00000000000..ce5e33261a9 --- /dev/null +++ b/spec/models/concerns/loose_foreign_key_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKey do + let(:project_klass) do + Class.new(ApplicationRecord) do + include LooseForeignKey + + self.table_name = 'projects' + + loose_foreign_key :issues, :project_id, on_delete: :async_delete, gitlab_schema: :gitlab_main + loose_foreign_key 'merge_requests', 'project_id', 'on_delete' => 'async_nullify', 'gitlab_schema' => :gitlab_main + end + end + + it 'exposes the loose foreign key definitions' do + definitions = project_klass.loose_foreign_key_definitions + + tables = definitions.map(&:to_table) + expect(tables).to eq(%w[issues merge_requests]) + end + + it 'casts strings to symbol' do + definition = project_klass.loose_foreign_key_definitions.last + + expect(definition.from_table).to eq('projects') + expect(definition.to_table).to eq('merge_requests') + expect(definition.column).to eq('project_id') + expect(definition.on_delete).to eq(:async_nullify) + expect(definition.options[:gitlab_schema]).to eq(:gitlab_main) + end + + context 'validation' do + context 'on_delete validation' do + let(:invalid_class) do + Class.new(ApplicationRecord) do + include LooseForeignKey + + self.table_name = 'projects' + + loose_foreign_key :issues, :project_id, on_delete: :async_delete, gitlab_schema: :gitlab_main + loose_foreign_key :merge_requests, :project_id, on_delete: :async_nullify, gitlab_schema: :gitlab_main + loose_foreign_key :merge_requests, :project_id, on_delete: :destroy, gitlab_schema: :gitlab_main + end + end + + it 'raises error when invalid `on_delete` option was given' do + expect { invalid_class }.to raise_error /Invalid on_delete option given: destroy/ + end + end + + context 'gitlab_schema validation' do + let(:invalid_class) do + Class.new(ApplicationRecord) do + include LooseForeignKey + + self.table_name = 'projects' + + loose_foreign_key :merge_requests, :project_id, on_delete: :async_nullify, gitlab_schema: :unknown + end + end + + it 'raises error when invalid `gitlab_schema` option was given' do + expect { invalid_class }.to raise_error /Invalid gitlab_schema option given: unknown/ + end + end + + context 'inheritance validation' do + let(:inherited_project_class) do + Class.new(Project) do + include LooseForeignKey + + loose_foreign_key :issues, :project_id, on_delete: :async_delete, gitlab_schema: :gitlab_main + end + end + + it 'raises error when loose_foreign_key is defined in a child ActiveRecord model' do + expect { inherited_project_class }.to raise_error /Please define the loose_foreign_key on the Project class/ + end + end + end +end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 53561586d61..7fb6de8b77e 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -135,6 +135,20 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do environment.stop end + + context 'when environment has auto stop period' do + let!(:environment) { create(:environment, :available, :auto_stoppable, project: project) } + + it 'clears auto stop period when the environment has stopped' do + environment.stop! + + expect(environment.auto_stop_at).to be_nil + end + + it 'does not clear auto stop period when the environment has not stopped' do + expect(environment.auto_stop_at).to be_present + end + end end describe '.for_name_like' do @@ -233,55 +247,6 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do end end - describe '.stop_actions' do - subject { environments.stop_actions } - - let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { create(:user) } - - let(:environments) { Environment.all } - - before_all do - project.add_developer(user) - project.repository.add_branch(user, 'review/feature-1', 'master') - project.repository.add_branch(user, 'review/feature-2', 'master') - end - - shared_examples_for 'correct filtering' do - it 'returns stop actions for available environments only' do - expect(subject.count).to eq(1) - expect(subject.first.name).to eq('stop_review_app') - expect(subject.first.ref).to eq('review/feature-1') - end - end - - before do - create_review_app(user, project, 'review/feature-1') - create_review_app(user, project, 'review/feature-2') - end - - it 'returns stop actions for environments' do - expect(subject.count).to eq(2) - expect(subject).to match_array(Ci::Build.where(name: 'stop_review_app')) - end - - context 'when one of the stop actions has already been executed' do - before do - Ci::Build.where(ref: 'review/feature-2').find_by_name('stop_review_app').enqueue! - end - - it_behaves_like 'correct filtering' - end - - context 'when one of the deployments does not have stop action' do - before do - Deployment.where(ref: 'review/feature-2').update_all(on_stop: nil) - end - - it_behaves_like 'correct filtering' - end - end - describe '.pluck_names' do subject { described_class.pluck_names } diff --git a/spec/services/environments/auto_stop_service_spec.rb b/spec/services/environments/auto_stop_service_spec.rb index 93b1596586f..8dad59cbefd 100644 --- a/spec/services/environments/auto_stop_service_spec.rb +++ b/spec/services/environments/auto_stop_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Environments::AutoStopService, :clean_gitlab_redis_shared_state do +RSpec.describe Environments::AutoStopService, :clean_gitlab_redis_shared_state, :sidekiq_inline do include CreateEnvironmentsHelpers include ExclusiveLeaseHelpers @@ -42,6 +42,15 @@ RSpec.describe Environments::AutoStopService, :clean_gitlab_redis_shared_state d expect(Ci::Build.where(name: 'stop_review_app').map(&:status).uniq).to eq(['pending']) end + it 'schedules stop processes in bulk' do + args = [[Environment.find_by_name('review/feature-1').id], [Environment.find_by_name('review/feature-2').id]] + + expect(Environments::AutoStopWorker) + .to receive(:bulk_perform_async).with(args).once.and_call_original + + subject + end + context 'when the other sidekiq worker has already been running' do before do stub_exclusive_lease_taken(described_class::EXCLUSIVE_LOCK_KEY) diff --git a/spec/services/environments/stop_service_spec.rb b/spec/services/environments/stop_service_spec.rb index 52be512612d..acc9869002f 100644 --- a/spec/services/environments/stop_service_spec.rb +++ b/spec/services/environments/stop_service_spec.rb @@ -237,60 +237,6 @@ RSpec.describe Environments::StopService do end end - describe '.execute_in_batch' do - subject { described_class.execute_in_batch(environments) } - - let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { create(:user) } - - let(:environments) { Environment.available } - - before_all do - project.add_developer(user) - project.repository.add_branch(user, 'review/feature-1', 'master') - project.repository.add_branch(user, 'review/feature-2', 'master') - end - - before do - create_review_app(user, project, 'review/feature-1') - create_review_app(user, project, 'review/feature-2') - end - - it 'stops environments' do - expect { subject } - .to change { project.environments.all.map(&:state).uniq } - .from(['available']).to(['stopped']) - - expect(project.environments.all.map(&:auto_stop_at).uniq).to eq([nil]) - end - - it 'plays stop actions' do - expect { subject } - .to change { Ci::Build.where(name: 'stop_review_app').map(&:status).uniq } - .from(['manual']).to(['pending']) - end - - context 'when user does not have a permission to play the stop action' do - before do - project.team.truncate - end - - it 'tracks the exception' do - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with(Gitlab::Access::AccessDeniedError, anything) - .twice - .and_call_original - - subject - end - - after do - project.add_developer(user) - end - end - end - def expect_environment_stopped_on(branch) expect { service.execute_for_branch(branch) } .to change { Environment.last.state }.from('available').to('stopped') diff --git a/spec/workers/environments/auto_stop_worker_spec.rb b/spec/workers/environments/auto_stop_worker_spec.rb new file mode 100644 index 00000000000..1983cfa18ea --- /dev/null +++ b/spec/workers/environments/auto_stop_worker_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Environments::AutoStopWorker do + include CreateEnvironmentsHelpers + + subject { worker.perform(environment_id) } + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + + before_all do + project.repository.add_branch(developer, 'review/feature', 'master') + end + + let!(:environment) { create_review_app(user, project, 'review/feature').environment } + let(:environment_id) { environment.id } + let(:worker) { described_class.new } + let(:user) { developer } + + it 'stops the environment' do + expect { subject } + .to change { Environment.find_by_name('review/feature').state } + .from('available').to('stopped') + end + + it 'executes the stop action' do + expect { subject } + .to change { Ci::Build.find_by_name('stop_review_app').status } + .from('manual').to('pending') + end + + context 'when user does not have a permission to play the stop action' do + let(:user) { reporter } + + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + + context 'when the environment has already been stopped' do + before do + environment.stop! + end + + it 'does not execute the stop action' do + expect { subject } + .not_to change { Ci::Build.find_by_name('stop_review_app').status } + end + end + + context 'when there are no deployments and associted stop actions' do + let!(:environment) { create(:environment) } + + it 'stops the environment' do + subject + + expect(environment.reload).to be_stopped + end + end + + context 'when there are no corresponding environment record' do + let!(:environment) { double(:environment, id: non_existing_record_id) } + + it 'ignores the invalid record' do + expect { subject }.not_to raise_error + end + end +end