diff options
-rw-r--r-- | app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue | 1 | ||||
-rw-r--r-- | app/models/deployment.rb | 5 | ||||
-rw-r--r-- | app/services/deployments/link_merge_requests_service.rb | 15 | ||||
-rw-r--r-- | changelogs/unreleased/link-mrs-failed-deploys.yml | 5 | ||||
-rw-r--r-- | doc/development/code_review.md | 16 | ||||
-rw-r--r-- | doc/development/elasticsearch.md | 10 | ||||
-rw-r--r-- | doc/development/testing_guide/end_to_end/best_practices.md | 9 | ||||
-rw-r--r-- | doc/development/usage_ping/index.md | 26 | ||||
-rw-r--r-- | qa/qa/page/merge_request/show.rb | 6 | ||||
-rw-r--r-- | qa/qa/runtime/env.rb | 4 | ||||
-rw-r--r-- | qa/qa/runtime/user.rb | 2 | ||||
-rw-r--r-- | qa/qa/specs/features/browser_ui/3_create/merge_request/revert_spec.rb | 39 | ||||
-rw-r--r-- | spec/models/deployment_spec.rb | 8 | ||||
-rw-r--r-- | spec/services/deployments/link_merge_requests_service_spec.rb | 11 |
14 files changed, 135 insertions, 22 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue index ae127fa66db..9da3bea9362 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue @@ -130,6 +130,7 @@ export default { size="small" category="secondary" variant="warning" + data-qa-selector="revert_button" @click="openRevertModal" > {{ revertLabel }} diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 446e6174df2..9bcc6fd90b2 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -94,11 +94,6 @@ class Deployment < ApplicationRecord after_transition any => :success do |deployment| deployment.run_after_commit do Deployments::UpdateEnvironmentWorker.perform_async(id) - end - end - - after_transition any => FINISHED_STATUSES do |deployment| - deployment.run_after_commit do Deployments::LinkMergeRequestWorker.perform_async(id) end end diff --git a/app/services/deployments/link_merge_requests_service.rb b/app/services/deployments/link_merge_requests_service.rb index eba5082e6c3..0ac572def8f 100644 --- a/app/services/deployments/link_merge_requests_service.rb +++ b/app/services/deployments/link_merge_requests_service.rb @@ -18,6 +18,21 @@ module Deployments # app deployments, as this is not useful. return if deployment.environment.environment_type + # This service is triggered by a Sidekiq worker, which only runs when a + # deployment is successful. We add an extra check here in case we ever + # call this service elsewhere and forget to check the status there. + # + # The reason we only want to link successful deployments is as follows: + # when we link a merge request, we don't link it to future deployments for + # the same environment. If we were to link an MR to a failed deploy, we + # wouldn't be able to later on link it to a successful deploy (e.g. after + # the deploy is retried). + # + # In addition, showing failed deploys in the UI of a merge request isn't + # useful to users, as they can't act upon the information in any + # meaningful way (i.e. they can't just retry the deploy themselves). + return unless deployment.success? + if (prev = deployment.previous_environment_deployment) link_merge_requests_for_range(prev.sha, deployment.sha) else diff --git a/changelogs/unreleased/link-mrs-failed-deploys.yml b/changelogs/unreleased/link-mrs-failed-deploys.yml new file mode 100644 index 00000000000..7271eaf3d31 --- /dev/null +++ b/changelogs/unreleased/link-mrs-failed-deploys.yml @@ -0,0 +1,5 @@ +--- +title: Only link merge requests to successful deployments +merge_request: 58072 +author: +type: fixed diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 03ebd333e28..3e069eba2f6 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -100,8 +100,9 @@ with [domain expertise](#domain-experts). Read the [database review guidelines](database_review.md) for more details. 1. If your merge request includes frontend changes (*1*), it must be **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_frontend)**. -1. If your merge request includes UX changes (*1*), it must be - **approved by a [UX team member](https://about.gitlab.com/company/team/)**. +1. If your merge request includes user-facing changes (*3*), it must be + **approved by a [Product Designer](https://about.gitlab.com/handbook/engineering/ux/product-design/)**, + based on assignments in the appropriate [DevOps stage group](https://about.gitlab.com/handbook/product/categories/#devops-stages). 1. If your merge request includes adding a new JavaScript library (*1*)... - If the library significantly increases the [bundle size](https://gitlab.com/gitlab-org/frontend/playground/webpack-memory-metrics/-/blob/master/doc/report.md), it must @@ -110,16 +111,14 @@ with [domain expertise](#domain-experts). GitLab, the license must be **approved by a [legal department member](https://about.gitlab.com/handbook/legal/)**. More information about license compatibility can be found in our [GitLab Licensing and Compatibility documentation](licensing.md). -1. If your merge request includes adding a new UI/UX paradigm (*1*), it must be - **approved by a [UX lead](https://about.gitlab.com/company/team/)**. 1. If your merge request includes a new dependency or a file system change, it must be **approved by a [Distribution team member](https://about.gitlab.com/company/team/)**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/development/enablement/distribution/#how-to-work-with-distribution) for more details. 1. If your merge request includes documentation changes, it must be **approved by a [Technical writer](https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments)**, based on the appropriate [product category](https://about.gitlab.com/handbook/product/categories/). -1. If your merge request includes end-to-end **and** non-end-to-end changes (*3*), it must be **approved +1. If your merge request includes end-to-end **and** non-end-to-end changes (*4*), it must be **approved by a [Software Engineer in Test](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors)**. -1. If your merge request only includes end-to-end changes (*3*) **or** if the MR author is a [Software Engineer in Test](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors), it must be **approved by a [Quality maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_qa)** +1. If your merge request only includes end-to-end changes (*4*) **or** if the MR author is a [Software Engineer in Test](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors), it must be **approved by a [Quality maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_qa)** 1. If your merge request includes a new or updated [application limit](https://about.gitlab.com/handbook/product/product-processes/#introducing-application-limits), it must be **approved by a [product manager](https://about.gitlab.com/company/team/)**. 1. If your merge request includes Product Intelligence (telemetry or analytics) changes, it should be reviewed and approved by a [Product Intelligence engineer](https://gitlab.com/gitlab-org/growth/product_intelligence/engineers). 1. If your merge request includes an addition of, or changes to a [Feature spec](testing_guide/testing_levels.md#frontend-feature-tests), it must be **approved by a [Quality maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_qa) or [Quality reviewer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_reviewers_qa)**. @@ -129,7 +128,10 @@ with [domain expertise](#domain-experts). - (*2*): We encourage you to seek guidance from a database maintainer if your merge request is potentially introducing expensive queries. It is most efficient to comment on the line of code in question with the SQL queries so they can give their advice. -- (*3*): End-to-end changes include all files within the `qa` directory. +- (*3*): User-facing changes include both visual changes (regardless of how minor), + and changes to the rendered DOM which impact how a screen reader may announce + the content. +- (*4*): End-to-end changes include all files within the `qa` directory. #### Security requirements diff --git a/doc/development/elasticsearch.md b/doc/development/elasticsearch.md index 3f14ca454fe..ba977b28247 100644 --- a/doc/development/elasticsearch.md +++ b/doc/development/elasticsearch.md @@ -198,7 +198,10 @@ filename format, which is similar to Rails database migrations: # frozen_string_literal: true class MigrationName < Elastic::Migration - # Important: Any update to the Elastic index mappings should be replicated in Elastic::Latest::Config + # Important: Any updates to the Elastic index mappings must be replicated in the respective + # configuration files: + # - `Elastic::Latest::Config`, for the main index. + # - `Elastic::Latest::<Type>Config`, for standalone indices. def migrate end @@ -214,7 +217,10 @@ Applied migrations are stored in `gitlab-#{RAILS_ENV}-migrations` index. All mig are applied by the [`Elastic::MigrationWorker`](https://gitlab.com/gitlab-org/gitlab/blob/master/ee/app/workers/elastic/migration_worker.rb) cron worker sequentially. -Any update to the Elastic index mappings should be replicated in [`Elastic::Latest::Config`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/elastic/latest/config.rb). +To update Elastic index mappings, apply the configuration to the respective files: + +- For the main index: [`Elastic::Latest::Config`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/elastic/latest/config.rb). +- For standalone indices: `Elastic::Latest::<Type>Config`. Migrations can be built with a retry limit and have the ability to be [failed and marked as halted](https://gitlab.com/gitlab-org/gitlab/-/blob/66e899b6637372a4faf61cfd2f254cbdd2fb9f6d/ee/lib/elastic/migration.rb#L40). Any data or index cleanup needed to support migration retries should be handled within the migration. diff --git a/doc/development/testing_guide/end_to_end/best_practices.md b/doc/development/testing_guide/end_to_end/best_practices.md index 2b4212a0172..15520d8a6b1 100644 --- a/doc/development/testing_guide/end_to_end/best_practices.md +++ b/doc/development/testing_guide/end_to_end/best_practices.md @@ -223,6 +223,15 @@ In summary: - **Do**: Split tests across separate files, unless the tests share expensive setup. - **Don't**: Put new tests in an existing file without considering the impact on parallelization. +## `let` variables vs instance variables + +By default, follow the [testing best practices](../best_practices.md#subject-and-let-variables) when using `let` +or instance variables. However, in end-to-end tests, set-ups such as creating resources are expensive. +If you use `let` to store a resource, it will be created for each example separately. +If the resource can be shared among multiple examples, use an instance variable in the `before(:all)` +block instead of `let` to save run time. +When the variable cannot be shared by multiple examples, use `let`. + ## Limit the use of the UI in `before(:context)` and `after` hooks Limit the use of `before(:context)` hooks to perform setup tasks with only API calls, diff --git a/doc/development/usage_ping/index.md b/doc/development/usage_ping/index.md index c41510fa98a..7e4dab8b860 100644 --- a/doc/development/usage_ping/index.md +++ b/doc/development/usage_ping/index.md @@ -214,11 +214,12 @@ For GitLab.com, there are extremely large tables with 15 second query timeouts, | `merge_request_diff_files` | 1082 | | `events` | 514 | -We have several batch counting methods available: +The following operation methods are available for your use: - [Ordinary Batch Counters](#ordinary-batch-counters) - [Distinct Batch Counters](#distinct-batch-counters) -- [Sum Batch Counters](#sum-batch-counters) +- [Sum Batch Operation](#sum-batch-operation) +- [Add Operation](#add-operation) - [Estimated Batch Counters](#estimated-batch-counters) Batch counting requires indexes on columns to calculate max, min, and range queries. In some cases, @@ -276,7 +277,7 @@ distinct_count(::Note.with_suggestions.where(time_period), :author_id, start: :: distinct_count(::Clusters::Applications::CertManager.where(time_period).available.joins(:cluster), 'clusters.user_id') ``` -### Sum Batch Counters +### Sum Batch Operation Handles `ActiveRecord::StatementInvalid` error @@ -317,6 +318,25 @@ sum(Issue.group(:state_id), :weight)) # returns => {1=>3542, 2=>6820} ``` +### Add Operation + +Handles `StandardError`. + +Returns `-1` if any of the arguments are `-1`. + +Sum the values given as parameters. + +Method: `add(*args)` + +Examples + +```ruby +project_imports = distinct_count(::Project.where.not(import_type: nil), :creator_id) +bulk_imports = distinct_count(::BulkImport, :user_id) + + add(project_imports, bulk_imports) +``` + ### Estimated Batch Counters > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48233) in GitLab 13.7. diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index 4cf9e404bbf..758bfdabf94 100644 --- a/qa/qa/page/merge_request/show.rb +++ b/qa/qa/page/merge_request/show.rb @@ -108,6 +108,7 @@ module QA end view 'app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue' do + element :revert_button element :cherry_pick_button end @@ -368,6 +369,11 @@ module QA click_element(:cherry_pick_button, Page::Component::CommitModal) click_element(:submit_commit_button) end + + def revert_change! + click_element(:revert_button, Page::Component::CommitModal) + click_element(:submit_commit_button) + end end end end diff --git a/qa/qa/runtime/env.rb b/qa/qa/runtime/env.rb index 0e384a99fcf..e4b92dc2e0d 100644 --- a/qa/qa/runtime/env.rb +++ b/qa/qa/runtime/env.rb @@ -144,6 +144,10 @@ module QA ENV['GITLAB_PASSWORD'] end + def initial_root_password + ENV['GITLAB_INITIAL_ROOT_PASSWORD'] + end + def github_username ENV['GITHUB_USERNAME'] end diff --git a/qa/qa/runtime/user.rb b/qa/qa/runtime/user.rb index c50fcc25304..a836206034d 100644 --- a/qa/qa/runtime/user.rb +++ b/qa/qa/runtime/user.rb @@ -18,7 +18,7 @@ module QA end def default_password - '5iveL!fe' + Runtime::Env.initial_root_password || '5iveL!fe' end def username diff --git a/qa/qa/specs/features/browser_ui/3_create/merge_request/revert_spec.rb b/qa/qa/specs/features/browser_ui/3_create/merge_request/revert_spec.rb new file mode 100644 index 00000000000..3574cdbe4ac --- /dev/null +++ b/qa/qa/specs/features/browser_ui/3_create/merge_request/revert_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module QA + RSpec.describe 'Create' do + describe 'Merged merge request' do + let(:project) do + Resource::Project.fabricate_via_api! do |project| + project.name = 'revert' + end + end + + let(:revertable_merge_request) do + Resource::MergeRequest.fabricate_via_api! do |merge_request| + merge_request.project = project + end + end + + before do + Flow::Login.sign_in + end + + it 'can be reverted', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1745' do + revertable_merge_request.visit! + + Page::MergeRequest::Show.perform do |merge_request| + merge_request.merge! + merge_request.revert_change! + end + + Page::MergeRequest::New.perform(&:create_merge_request) + + Page::MergeRequest::Show.perform do |merge_request| + merge_request.click_diffs_tab + expect(merge_request).to have_file(revertable_merge_request.file_name) + end + end + end + end +end diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index 49c3c97e7df..aae0cedc079 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -161,9 +161,9 @@ RSpec.describe Deployment do end end - it 'executes Deployments::LinkMergeRequestWorker asynchronously' do + it 'does not execute Deployments::LinkMergeRequestWorker' do expect(Deployments::LinkMergeRequestWorker) - .to receive(:perform_async).with(deployment.id) + .not_to receive(:perform_async).with(deployment.id) deployment.drop! end @@ -188,9 +188,9 @@ RSpec.describe Deployment do end end - it 'executes Deployments::LinkMergeRequestWorker asynchronously' do + it 'does not execute Deployments::LinkMergeRequestWorker' do expect(Deployments::LinkMergeRequestWorker) - .to receive(:perform_async).with(deployment.id) + .not_to receive(:perform_async).with(deployment.id) deployment.cancel! end diff --git a/spec/services/deployments/link_merge_requests_service_spec.rb b/spec/services/deployments/link_merge_requests_service_spec.rb index e2ac2273b8c..a5a13230d6f 100644 --- a/spec/services/deployments/link_merge_requests_service_spec.rb +++ b/spec/services/deployments/link_merge_requests_service_spec.rb @@ -32,6 +32,17 @@ RSpec.describe Deployments::LinkMergeRequestsService do end end + context 'when the deployment failed' do + it 'does nothing' do + environment = create(:environment, name: 'foo') + deploy = create(:deployment, :failed, environment: environment) + + expect(deploy).not_to receive(:link_merge_requests) + + described_class.new(deploy).execute + end + end + context 'when there is a previous deployment' do it 'links all merge requests merged since the previous deployment' do deploy1 = create( |