Add latest changes from gitlab-org/gitlab@master

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(