Merge branch 'mc/feature/remove-keep-latest-artifact-ff' into 'master'

Remove keep latest artifact feature flags

See merge request gitlab-org/gitlab!40478

app/models/ci/ref.rb

@@ -33,8 +33,6 @@ module Ci
       state :still_failing, value: 5
 
       after_transition any => [:fixed, :success] do |ci_ref|
-        next unless ::Gitlab::Ci::Features.keep_latest_artifacts_for_ref_enabled?(ci_ref.project)
-
         ci_ref.run_after_commit do
           Ci::PipelineSuccessUnlockArtifactsWorker.perform_async(ci_ref.last_finished_pipeline_id)
         end

app/services/ci/destroy_expired_job_artifacts_service.rb

@@ -28,7 +28,7 @@ module Ci
     private
 
     def destroy_batch(klass)
-      artifact_batch = if klass == Ci::JobArtifact && Gitlab::Ci::Features.destroy_only_unlocked_expired_artifacts_enabled?
+      artifact_batch = if klass == Ci::JobArtifact
                          klass.expired(BATCH_SIZE).unlocked
                        else
                          klass.expired(BATCH_SIZE)

changelogs/unreleased/mc-feature-remove-keep-latest-artifact-ff.yml

+---
+title: Remove keep latest artifact feature flags.
+merge_request: 40478
+author:
+type: other

db/migrate/20200826073745_add_default_to_ci_pipeline_locked.rb

+# frozen_string_literal: true
+
+class AddDefaultToCiPipelineLocked < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  ARTIFACTS_LOCKED = 1
+  UNLOCKED = 0
+
+  def up
+    with_lock_retries do
+      change_column_default :ci_pipelines, :locked, ARTIFACTS_LOCKED
+    end
+  end
+
+  def down
+    with_lock_retries do
+      change_column_default :ci_pipelines, :locked, UNLOCKED
+    end
+  end
+end

db/post_migrate/20200826121552_remove_ci_job_artifacts_locked.rb

+# frozen_string_literal: true
+
+class RemoveCiJobArtifactsLocked < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      remove_column :ci_job_artifacts, :locked
+    end
+  end
+
+  def down
+    with_lock_retries do
+      add_column :ci_job_artifacts, :locked, :boolean
+    end
+  end
+end

db/schema_migrations/20200826073745

+d3b15469120ed213363de33a4b268ed71a710c40f02d4a669edf2c5412907209
\ No newline at end of file

db/schema_migrations/20200826121552

+8667c30042b19428b97e0995821c183e69f73394503c83a55ba7bd870df7c3e8
\ No newline at end of file

db/structure.sql

@@ -10083,7 +10083,6 @@ CREATE TABLE public.ci_job_artifacts (
     file_sha256 bytea,
     file_format smallint,
     file_location smallint,
-    locked boolean,
     CONSTRAINT check_27f0f6dbab CHECK ((file_store IS NOT NULL))
 );
 
@@ -10267,7 +10266,7 @@ CREATE TABLE public.ci_pipelines (
     target_sha bytea,
     external_pull_request_id bigint,
     ci_ref_id bigint,
-    locked smallint DEFAULT 0 NOT NULL,
+    locked smallint DEFAULT 1 NOT NULL,
     CONSTRAINT check_d7e99a025e CHECK ((lock_version IS NOT NULL))
 );
 

doc/ci/yaml/README.md

@@ -3289,10 +3289,10 @@ job:
 ```
 
 NOTE: **Note:**
-Since [GitLab 13.0](https://gitlab.com/gitlab-org/gitlab/-/issues/16267), the latest
-artifacts for refs can be locked against deletion, and kept regardless of the expiry time. This feature is disabled
-by default and is not ready for production use. It can be enabled for testing by
-enabling the `:keep_latest_artifact_for_ref` and `:destroy_only_unlocked_expired_artifacts` [feature flags](../../administration/feature_flags.md).
+The latest artifacts for refs are locked against deletion, and kept regardless of
+the expiry time. [Introduced in](https://gitlab.com/gitlab-org/gitlab/-/issues/16267)
+GitLab 13.0 behind a disabled feature flag, and [made the default behavior](https://gitlab.com/gitlab-org/gitlab/-/issues/229936)
+in GitLab 13.4.
 
 #### `artifacts:reports`
 

lib/gitlab/ci/features.rb

@@ -40,14 +40,6 @@ module Gitlab
         ::Feature.enabled?(:ci_raise_job_rules_without_workflow_rules_warning, default_enabled: true)
       end
 
-      def self.keep_latest_artifacts_for_ref_enabled?(project)
-        ::Feature.enabled?(:keep_latest_artifacts_for_ref, project, default_enabled: true)
-      end
-
-      def self.destroy_only_unlocked_expired_artifacts_enabled?
-        ::Feature.enabled?(:destroy_only_unlocked_expired_artifacts, default_enabled: true)
-      end
-
       def self.bulk_insert_on_create?(project)
         ::Feature.enabled?(:ci_bulk_insert_on_create, project, default_enabled: true)
       end

lib/gitlab/ci/pipeline/chain/build.rb

@@ -20,11 +20,7 @@ module Gitlab
               pipeline_schedule: @command.schedule,
               merge_request: @command.merge_request,
               external_pull_request: @command.external_pull_request,
-              variables_attributes: Array(@command.variables_attributes),
-              # This should be removed and set on the database column default
-              # level when the keep_latest_artifacts_for_ref feature flag is
-              # removed.
-              locked: ::Gitlab::Ci::Features.keep_latest_artifacts_for_ref_enabled?(@command.project) ? :artifacts_locked : :unlocked
+              variables_attributes: Array(@command.variables_attributes)
             )
           end
 

qa/qa/specs/features/ee/browser_ui/4_verify/locked_artifacts_spec.rb

@@ -4,8 +4,6 @@ module QA
   RSpec.describe 'Verify', :docker, :runner, :requires_admin do
     describe 'Artifacts' do
       context 'when locked' do
-        let(:ff_keep_latest) { 'keep_latest_artifacts_for_ref' }
-        let(:ff_destroy_unlocked) { 'destroy_only_unlocked_expired_artifacts' }
         let(:file_name) { 'artifact.txt' }
         let(:directory_name) { 'my_artifacts' }
         let(:executor) { "qa-runner-#{Time.now.to_i}" }
@@ -25,12 +23,10 @@ module QA
         end
 
         before do
-          [ff_keep_latest, ff_destroy_unlocked].each { |flag| Runtime::Feature.enable_and_verify(flag) }
           Flow::Login.sign_in
         end
 
         after do
-          [ff_keep_latest, ff_destroy_unlocked].each { |flag| Runtime::Feature.disable_and_verify(flag) }
           runner.remove_via_api!
         end
 

spec/models/ci/ref_spec.rb

@@ -16,51 +16,33 @@ RSpec.describe Ci::Ref do
         stub_const('Ci::PipelineSuccessUnlockArtifactsWorker', unlock_artifacts_worker_spy)
       end
 
-      context 'when keep latest artifact feature is enabled' do
-        before do
-          stub_feature_flags(keep_latest_artifacts_for_ref: true)
-        end
-
-        where(:initial_state, :action, :count) do
-          :unknown | :succeed! | 1
-          :unknown | :do_fail! | 0
-          :success | :succeed! | 1
-          :success | :do_fail! | 0
-          :failed | :succeed! | 1
-          :failed | :do_fail! | 0
-          :fixed | :succeed! | 1
-          :fixed | :do_fail! | 0
-          :broken | :succeed! | 1
-          :broken | :do_fail! | 0
-          :still_failing | :succeed | 1
-          :still_failing | :do_fail | 0
-        end
-
-        with_them do
-          context "when transitioning states" do
-            before do
-              status_value = Ci::Ref.state_machines[:status].states[initial_state].value
-              ci_ref.update!(status: status_value)
-            end
-
-            it 'calls unlock artifacts service' do
-              ci_ref.send(action)
-
-              expect(unlock_artifacts_worker_spy).to have_received(:perform_async).exactly(count).times
-            end
-          end
-        end
+      where(:initial_state, :action, :count) do
+        :unknown | :succeed! | 1
+        :unknown | :do_fail! | 0
+        :success | :succeed! | 1
+        :success | :do_fail! | 0
+        :failed | :succeed! | 1
+        :failed | :do_fail! | 0
+        :fixed | :succeed! | 1
+        :fixed | :do_fail! | 0
+        :broken | :succeed! | 1
+        :broken | :do_fail! | 0
+        :still_failing | :succeed | 1
+        :still_failing | :do_fail | 0
       end
 
-      context 'when keep latest artifact feature is not enabled' do
-        before do
-          stub_feature_flags(keep_latest_artifacts_for_ref: false)
-        end
+      with_them do
+        context "when transitioning states" do
+          before do
+            status_value = Ci::Ref.state_machines[:status].states[initial_state].value
+            ci_ref.update!(status: status_value)
+          end
 
-        it 'does not call unlock artifacts service' do
-          ci_ref.succeed!
+          it 'calls unlock artifacts service' do
+            ci_ref.send(action)
 
-          expect(unlock_artifacts_worker_spy).not_to have_received(:perform_async)
+            expect(unlock_artifacts_worker_spy).to have_received(:perform_async).exactly(count).times
+          end
         end
       end
     end