Merge branch 'fix-default-value-for-retried' into 'master'

Fix retried value for commit statuses [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!54300

app/models/commit_status.rb

@@ -84,6 +84,8 @@ class CommitStatus < ApplicationRecord
   # extend this `Hash` with new values.
   enum_with_nil failure_reason: Enums::Ci::CommitStatus.failure_reasons
 
+  default_value_for :retried, false
+
   ##
   # We still create some CommitStatuses outside of CreatePipelineService.
   #
@@ -290,6 +292,14 @@ class CommitStatus < ApplicationRecord
     failed? && !unrecoverable_failure?
   end
 
+  def update_older_statuses_retried!
+    self.class
+      .latest
+      .where(name: name)
+      .where.not(id: id)
+      .update_all(retried: true, processed: true)
+  end
+
   private
 
   def unrecoverable_failure?

app/services/ci/process_pipeline_service.rb

@@ -30,6 +30,8 @@ module Ci
     # this updates only when there are data that needs to be updated, there are two groups with no retried flag
     # rubocop: disable CodeReuse/ActiveRecord
     def update_retried
+      return if Feature.enabled?(:ci_remove_update_retried_from_process_pipeline, pipeline.project, default_enabled: :yaml)
+
       # find the latest builds for each name
       latest_statuses = pipeline.latest_statuses
         .group(:name)

app/services/projects/update_pages_service.rb

@@ -33,6 +33,7 @@ module Projects
       @status = create_status
       @status.enqueue!
       @status.run!
+      @status.update_older_statuses_retried! if Feature.enabled?(:ci_fix_commit_status_retried, project, default_enabled: :yaml)
 
       raise InvalidStateError, 'missing pages artifacts' unless build.artifacts?
       raise InvalidStateError, 'build SHA is outdated for this ref' unless latest?

config/feature_flags/development/ci_fix_commit_status_retried.yml

+---
+name: ci_fix_commit_status_retried
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54300
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/321631
+milestone: '13.9'
+type: development
+group: group::pipeline authoring
+default_enabled: false

config/feature_flags/development/ci_remove_update_retried_from_process_pipeline.yml

+---
+name: ci_remove_update_retried_from_process_pipeline
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54300
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/321630
+milestone: '13.9'
+type: development
+group: group::pipeline authoring
+default_enabled: false

lib/api/commit_statuses.rb

@@ -100,7 +100,12 @@ module API
           attributes_for_keys(%w[target_url description coverage])
 
         status.update(optional_attributes) if optional_attributes.any?
-        render_validation_error!(status) if status.invalid?
+
+        if status.valid?
+          status.update_older_statuses_retried! if Feature.enabled?(:ci_fix_commit_status_retried, user_project, default_enabled: :yaml)
+        else
+          render_validation_error!(status)
+        end
 
         begin
           case params[:state]

spec/models/ci/pipeline_spec.rb

@@ -1131,22 +1131,28 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
           end
 
           context 'when commit status is retried' do
-            before do
+            let!(:old_commit_status) do
               create(:commit_status, pipeline: pipeline,
-                                    stage: 'build',
-                                    name: 'mac',
-                                    stage_idx: 0,
-                                    status: 'success')
-
-              Ci::ProcessPipelineService
-                .new(pipeline)
-                .execute
+                                     stage: 'build',
+                                     name: 'mac',
+                                     stage_idx: 0,
+                                     status: 'success')
             end
 
-            it 'ignores the previous state' do
-              expect(statuses).to eq([%w(build success),
-                                      %w(test success),
-                                      %w(deploy running)])
+            context 'when FF ci_remove_update_retried_from_process_pipeline is disabled' do
+              before do
+                stub_feature_flags(ci_remove_update_retried_from_process_pipeline: false)
+
+                Ci::ProcessPipelineService
+                  .new(pipeline)
+                  .execute
+              end
+
+              it 'ignores the previous state' do
+                expect(statuses).to eq([%w(build success),
+                                        %w(test success),
+                                        %w(deploy running)])
+              end
             end
           end
         end

spec/requests/api/commit_statuses_spec.rb

@@ -291,7 +291,7 @@ RSpec.describe API::CommitStatuses do
       end
 
       context 'when retrying a commit status' do
-        before do
+        subject(:post_request) do
           post api(post_url, developer),
             params: { state: 'failed', name: 'test', ref: 'master' }
 
@@ -300,15 +300,45 @@ RSpec.describe API::CommitStatuses do
         end
 
         it 'correctly posts a new commit status' do
+          post_request
+
           expect(response).to have_gitlab_http_status(:created)
           expect(json_response['sha']).to eq(commit.id)
           expect(json_response['status']).to eq('success')
         end
 
-        it 'retries a commit status', :sidekiq_might_not_need_inline do
-          expect(CommitStatus.count).to eq 2
-          expect(CommitStatus.first).to be_retried
-          expect(CommitStatus.last.pipeline).to be_success
+        context 'feature flags' do
+          using RSpec::Parameterized::TableSyntax
+
+          where(:ci_fix_commit_status_retried, :ci_remove_update_retried_from_process_pipeline, :previous_statuses_retried) do
+            true  | true  | true
+            true  | false | true
+            false | true  | false
+            false | false | true
+          end
+
+          with_them do
+            before do
+              stub_feature_flags(
+                ci_fix_commit_status_retried: ci_fix_commit_status_retried,
+                ci_remove_update_retried_from_process_pipeline: ci_remove_update_retried_from_process_pipeline
+              )
+            end
+
+            it 'retries a commit status', :sidekiq_might_not_need_inline do
+              post_request
+
+              expect(CommitStatus.count).to eq 2
+
+              if previous_statuses_retried
+                expect(CommitStatus.first).to be_retried
+                expect(CommitStatus.last.pipeline).to be_success
+              else
+                expect(CommitStatus.first).not_to be_retried
+                expect(CommitStatus.last.pipeline).to be_failed
+              end
+            end
+          end
         end
       end
 

spec/services/ci/process_pipeline_service_spec.rb

@@ -43,42 +43,59 @@ RSpec.describe Ci::ProcessPipelineService do
     let!(:build) { create_build('build') }
     let!(:test) { create_build('test') }
 
-    it 'returns unique statuses' do
-      subject.execute
+    context 'when FF ci_remove_update_retried_from_process_pipeline is enabled' do
+      it 'does not update older builds as retried' do
+        subject.execute
 
-      expect(all_builds.latest).to contain_exactly(build, test)
-      expect(all_builds.retried).to contain_exactly(build_retried)
+        expect(all_builds.latest).to contain_exactly(build, build_retried, test)
+        expect(all_builds.retried).to be_empty
+      end
     end
 
-    context 'counter ci_legacy_update_jobs_as_retried_total' do
-      let(:counter) { double(increment: true) }
-
+    context 'when FF ci_remove_update_retried_from_process_pipeline is disabled' do
       before do
-        allow(Gitlab::Metrics).to receive(:counter).and_call_original
-        allow(Gitlab::Metrics).to receive(:counter)
-                                    .with(:ci_legacy_update_jobs_as_retried_total, anything)
-                                    .and_return(counter)
+        stub_feature_flags(ci_remove_update_retried_from_process_pipeline: false)
       end
 
-      it 'increments the counter' do
-        expect(counter).to receive(:increment)
-
+      it 'returns unique statuses' do
         subject.execute
+
+        expect(all_builds.latest).to contain_exactly(build, test)
+        expect(all_builds.retried).to contain_exactly(build_retried)
       end
 
-      context 'when the previous build has already retried column true' do
+      context 'counter ci_legacy_update_jobs_as_retried_total' do
+        let(:counter) { double(increment: true) }
+
         before do
-          build_retried.update_columns(retried: true)
+          allow(Gitlab::Metrics).to receive(:counter).and_call_original
+          allow(Gitlab::Metrics).to receive(:counter)
+                                      .with(:ci_legacy_update_jobs_as_retried_total, anything)
+                                      .and_return(counter)
         end
 
-        it 'does not increment the counter' do
-          expect(counter).not_to receive(:increment)
+        it 'increments the counter' do
+          expect(counter).to receive(:increment)
 
           subject.execute
         end
+
+        context 'when the previous build has already retried column true' do
+          before do
+            build_retried.update_columns(retried: true)
+          end
+
+          it 'does not increment the counter' do
+            expect(counter).not_to receive(:increment)
+
+            subject.execute
+          end
+        end
       end
     end
 
+    private
+
     def create_build(name, **opts)
       create(:ci_build, :created, pipeline: pipeline, name: name, **opts)
     end

spec/services/projects/update_pages_service_spec.rb

@@ -335,6 +335,41 @@ RSpec.describe Projects::UpdatePagesService do
     end
   end
 
+  context 'when retrying the job' do
+    let!(:older_deploy_job) do
+      create(:generic_commit_status, :failed, pipeline: pipeline,
+                                              ref: build.ref,
+                                              stage: 'deploy',
+                                              name: 'pages:deploy')
+    end
+
+    before do
+      create(:ci_job_artifact, :correct_checksum, file: file, job: build)
+      create(:ci_job_artifact, file_type: :metadata, file_format: :gzip, file: metadata, job: build)
+      build.reload
+    end
+
+    it 'marks older pages:deploy jobs retried' do
+      expect(execute).to eq(:success)
+
+      expect(older_deploy_job.reload).to be_retried
+    end
+
+    context 'when FF ci_fix_commit_status_retried is disabled' do
+      before do
+        stub_feature_flags(ci_fix_commit_status_retried: false)
+      end
+
+      it 'does not mark older pages:deploy jobs retried' do
+        expect(execute).to eq(:success)
+
+        expect(older_deploy_job.reload).not_to be_retried
+      end
+    end
+  end
+
+  private
+
   def deploy_status
     GenericCommitStatus.find_by(name: 'pages:deploy')
   end