Merge branch '340606-feature-flag-cleanup' into 'master'

Remove the container_registry_expiration_policies_caching feature flag

See merge request gitlab-org/gitlab!73737

app/services/projects/container_repository/cleanup_tags_service.rb

@@ -146,8 +146,7 @@ module Projects
 
       def caching_enabled?
         container_expiration_policy &&
-          older_than.present? &&
-          Feature.enabled?(:container_registry_expiration_policies_caching, @project)
+          older_than.present?
       end
 
       def throttling_enabled?

config/feature_flags/development/container_registry_expiration_policies_caching.yml

----
-name: container_registry_expiration_policies_caching
-introduced_by_url:
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/340606
-milestone: '14.3'
-type: development
-group: group::package
-default_enabled: false

spec/services/projects/container_repository/cleanup_tags_service_spec.rb

@@ -41,322 +41,320 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_
   describe '#execute' do
     subject { service.execute }
 
-    shared_examples 'reading and removing tags' do |caching_enabled: true|
-      context 'when no params are specified' do
-        let(:params) { {} }
+    context 'when no params are specified' do
+      let(:params) { {} }
 
-        it 'does not remove anything' do
-          expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService)
-            .not_to receive(:execute)
-          expect_no_caching
+      it 'does not remove anything' do
+        expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService)
+          .not_to receive(:execute)
+        expect_no_caching
 
-          is_expected.to eq(expected_service_response(before_truncate_size: 0, after_truncate_size: 0, before_delete_size: 0))
-        end
+        is_expected.to eq(expected_service_response(before_truncate_size: 0, after_truncate_size: 0, before_delete_size: 0))
       end
+    end
 
-      context 'when regex matching everything is specified' do
-        shared_examples 'removes all matches' do
-          it 'does remove all tags except latest' do
-            expect_no_caching
+    context 'when regex matching everything is specified' do
+      shared_examples 'removes all matches' do
+        it 'does remove all tags except latest' do
+          expect_no_caching
 
-            expect_delete(%w(A Ba Bb C D E))
+          expect_delete(%w(A Ba Bb C D E))
 
-            is_expected.to eq(expected_service_response(deleted: %w(A Ba Bb C D E)))
-          end
+          is_expected.to eq(expected_service_response(deleted: %w(A Ba Bb C D E)))
         end
+      end
+
+      let(:params) do
+        { 'name_regex_delete' => '.*' }
+      end
 
+      it_behaves_like 'removes all matches'
+
+      context 'with deprecated name_regex param' do
         let(:params) do
-          { 'name_regex_delete' => '.*' }
+          { 'name_regex' => '.*' }
         end
 
         it_behaves_like 'removes all matches'
+      end
+    end
 
-        context 'with deprecated name_regex param' do
-          let(:params) do
-            { 'name_regex' => '.*' }
-          end
+    context 'with invalid regular expressions' do
+      shared_examples 'handling an invalid regex' do
+        it 'keeps all tags' do
+          expect_no_caching
 
-          it_behaves_like 'removes all matches'
+          expect(Projects::ContainerRepository::DeleteTagsService)
+            .not_to receive(:new)
+
+          subject
         end
-      end
 
-      context 'with invalid regular expressions' do
-        shared_examples 'handling an invalid regex' do
-          it 'keeps all tags' do
-            expect_no_caching
+        it { is_expected.to eq(status: :error, message: 'invalid regex') }
 
-            expect(Projects::ContainerRepository::DeleteTagsService)
-              .not_to receive(:new)
+        it 'calls error tracking service' do
+          expect(Gitlab::ErrorTracking).to receive(:log_exception).and_call_original
 
-            subject
-          end
+          subject
+        end
+      end
 
-          it { is_expected.to eq(status: :error, message: 'invalid regex') }
+      context 'when name_regex_delete is invalid' do
+        let(:params) { { 'name_regex_delete' => '*test*' } }
 
-          it 'calls error tracking service' do
-            expect(Gitlab::ErrorTracking).to receive(:log_exception).and_call_original
+        it_behaves_like 'handling an invalid regex'
+      end
 
-            subject
-          end
-        end
+      context 'when name_regex is invalid' do
+        let(:params) { { 'name_regex' => '*test*' } }
 
-        context 'when name_regex_delete is invalid' do
-          let(:params) { { 'name_regex_delete' => '*test*' } }
+        it_behaves_like 'handling an invalid regex'
+      end
 
-          it_behaves_like 'handling an invalid regex'
-        end
+      context 'when name_regex_keep is invalid' do
+        let(:params) { { 'name_regex_keep' => '*test*' } }
 
-        context 'when name_regex is invalid' do
-          let(:params) { { 'name_regex' => '*test*' } }
+        it_behaves_like 'handling an invalid regex'
+      end
+    end
 
-          it_behaves_like 'handling an invalid regex'
-        end
+    context 'when delete regex matching specific tags is used' do
+      let(:params) do
+        { 'name_regex_delete' => 'C|D' }
+      end
 
-        context 'when name_regex_keep is invalid' do
-          let(:params) { { 'name_regex_keep' => '*test*' } }
+      it 'does remove C and D' do
+        expect_delete(%w(C D))
 
-          it_behaves_like 'handling an invalid regex'
-        end
+        expect_no_caching
+
+        is_expected.to eq(expected_service_response(deleted: %w(C D), before_truncate_size: 2, after_truncate_size: 2, before_delete_size: 2))
       end
 
-      context 'when delete regex matching specific tags is used' do
+      context 'with overriding allow regex' do
         let(:params) do
-          { 'name_regex_delete' => 'C|D' }
+          { 'name_regex_delete' => 'C|D',
+            'name_regex_keep' => 'C' }
         end
 
-        it 'does remove C and D' do
-          expect_delete(%w(C D))
+        it 'does not remove C' do
+          expect_delete(%w(D))
 
           expect_no_caching
 
-          is_expected.to eq(expected_service_response(deleted: %w(C D), before_truncate_size: 2, after_truncate_size: 2, before_delete_size: 2))
+          is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1))
         end
+      end
 
-        context 'with overriding allow regex' do
-          let(:params) do
-            { 'name_regex_delete' => 'C|D',
-              'name_regex_keep' => 'C' }
-          end
+      context 'with name_regex_delete overriding deprecated name_regex' do
+        let(:params) do
+          { 'name_regex' => 'C|D',
+            'name_regex_delete' => 'D' }
+        end
 
-          it 'does not remove C' do
-            expect_delete(%w(D))
+        it 'does not remove C' do
+          expect_delete(%w(D))
 
-            expect_no_caching
+          expect_no_caching
 
-            is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1))
-          end
+          is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1))
         end
+      end
+    end
 
-        context 'with name_regex_delete overriding deprecated name_regex' do
-          let(:params) do
-            { 'name_regex' => 'C|D',
-              'name_regex_delete' => 'D' }
-          end
+    context 'with allow regex value' do
+      let(:params) do
+        { 'name_regex_delete' => '.*',
+          'name_regex_keep' => 'B.*' }
+      end
 
-          it 'does not remove C' do
-            expect_delete(%w(D))
+      it 'does not remove B*' do
+        expect_delete(%w(A C D E))
 
-            expect_no_caching
+        expect_no_caching
 
-            is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1))
-          end
-        end
+        is_expected.to eq(expected_service_response(deleted: %w(A C D E), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 4))
+      end
+    end
+
+    context 'when keeping only N tags' do
+      let(:params) do
+        { 'name_regex' => 'A|B.*|C',
+          'keep_n' => 1 }
       end
 
-      context 'with allow regex value' do
-        let(:params) do
-          { 'name_regex_delete' => '.*',
-            'name_regex_keep' => 'B.*' }
-        end
+      it 'sorts tags by date' do
+        expect_delete(%w(Bb Ba C))
 
-        it 'does not remove B*' do
-          expect_delete(%w(A C D E))
+        expect_no_caching
 
-          expect_no_caching
+        expect(service).to receive(:order_by_date).and_call_original
 
-          is_expected.to eq(expected_service_response(deleted: %w(A C D E), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 4))
-        end
+        is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 3))
       end
+    end
 
-      context 'when keeping only N tags' do
-        let(:params) do
-          { 'name_regex' => 'A|B.*|C',
-            'keep_n' => 1 }
-        end
+    context 'when not keeping N tags' do
+      let(:params) do
+        { 'name_regex' => 'A|B.*|C' }
+      end
 
-        it 'sorts tags by date' do
-          expect_delete(%w(Bb Ba C))
+      it 'does not sort tags by date' do
+        expect_delete(%w(A Ba Bb C))
 
-          expect_no_caching
+        expect_no_caching
 
-          expect(service).to receive(:order_by_date).and_call_original
+        expect(service).not_to receive(:order_by_date)
 
-          is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 3))
-        end
+        is_expected.to eq(expected_service_response(deleted: %w(A Ba Bb C), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 4))
       end
+    end
 
-      context 'when not keeping N tags' do
-        let(:params) do
-          { 'name_regex' => 'A|B.*|C' }
-        end
-
-        it 'does not sort tags by date' do
-          expect_delete(%w(A Ba Bb C))
+    context 'when removing keeping only 3' do
+      let(:params) do
+        { 'name_regex_delete' => '.*',
+          'keep_n' => 3 }
+      end
 
-          expect_no_caching
+      it 'does remove B* and C as they are the oldest' do
+        expect_delete(%w(Bb Ba C))
 
-          expect(service).not_to receive(:order_by_date)
+        expect_no_caching
 
-          is_expected.to eq(expected_service_response(deleted: %w(A Ba Bb C), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 4))
-        end
+        is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
       end
+    end
 
-      context 'when removing keeping only 3' do
-        let(:params) do
-          { 'name_regex_delete' => '.*',
-            'keep_n' => 3 }
-        end
+    context 'when removing older than 1 day' do
+      let(:params) do
+        { 'name_regex_delete' => '.*',
+          'older_than' => '1 day' }
+      end
 
-        it 'does remove B* and C as they are the oldest' do
-          expect_delete(%w(Bb Ba C))
+      it 'does remove B* and C as they are older than 1 day' do
+        expect_delete(%w(Ba Bb C))
 
-          expect_no_caching
+        expect_no_caching
 
-          is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
-        end
+        is_expected.to eq(expected_service_response(deleted: %w(Ba Bb C), before_delete_size: 3))
       end
+    end
 
-      context 'when removing older than 1 day' do
-        let(:params) do
-          { 'name_regex_delete' => '.*',
-            'older_than' => '1 day' }
-        end
+    context 'when combining all parameters' do
+      let(:params) do
+        { 'name_regex_delete' => '.*',
+          'keep_n' => 1,
+          'older_than' => '1 day' }
+      end
 
-        it 'does remove B* and C as they are older than 1 day' do
-          expect_delete(%w(Ba Bb C))
+      it 'does remove B* and C' do
+        expect_delete(%w(Bb Ba C))
 
-          expect_no_caching
+        expect_no_caching
 
-          is_expected.to eq(expected_service_response(deleted: %w(Ba Bb C), before_delete_size: 3))
-        end
+        is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
       end
+    end
+
+    context 'when running a container_expiration_policy' do
+      let(:user) { nil }
 
-      context 'when combining all parameters' do
+      context 'with valid container_expiration_policy param' do
         let(:params) do
           { 'name_regex_delete' => '.*',
             'keep_n' => 1,
-            'older_than' => '1 day' }
+            'older_than' => '1 day',
+            'container_expiration_policy' => true }
         end
 
-        it 'does remove B* and C' do
-          expect_delete(%w(Bb Ba C))
+        it 'succeeds without a user' do
+          expect_delete(%w(Bb Ba C), container_expiration_policy: true)
 
-          expect_no_caching
+          expect_caching
 
           is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
         end
       end
 
-      context 'when running a container_expiration_policy' do
-        let(:user) { nil }
-
-        context 'with valid container_expiration_policy param' do
-          let(:params) do
-            { 'name_regex_delete' => '.*',
-              'keep_n' => 1,
-              'older_than' => '1 day',
-              'container_expiration_policy' => true }
-          end
-
-          it 'succeeds without a user' do
-            expect_delete(%w(Bb Ba C), container_expiration_policy: true)
-
-            caching_enabled ? expect_caching : expect_no_caching
-
-            is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
-          end
+      context 'without container_expiration_policy param' do
+        let(:params) do
+          { 'name_regex_delete' => '.*',
+            'keep_n' => 1,
+            'older_than' => '1 day' }
         end
 
-        context 'without container_expiration_policy param' do
-          let(:params) do
-            { 'name_regex_delete' => '.*',
-              'keep_n' => 1,
-              'older_than' => '1 day' }
-          end
-
-          it 'fails' do
-            is_expected.to eq(status: :error, message: 'access denied')
-          end
+        it 'fails' do
+          is_expected.to eq(status: :error, message: 'access denied')
         end
       end
+    end
 
-      context 'truncating the tags list' do
-        let(:params) do
-          {
-            'name_regex_delete' => '.*',
-            'keep_n' => 1
-          }
-        end
+    context 'truncating the tags list' do
+      let(:params) do
+        {
+          'name_regex_delete' => '.*',
+          'keep_n' => 1
+        }
+      end
 
-        shared_examples 'returning the response' do |status:, original_size:, before_truncate_size:, after_truncate_size:, before_delete_size:|
-          it 'returns the response' do
-            expect_no_caching
+      shared_examples 'returning the response' do |status:, original_size:, before_truncate_size:, after_truncate_size:, before_delete_size:|
+        it 'returns the response' do
+          expect_no_caching
 
-            result = subject
+          result = subject
 
-            service_response = expected_service_response(
-              status: status,
-              original_size: original_size,
-              before_truncate_size: before_truncate_size,
-              after_truncate_size: after_truncate_size,
-              before_delete_size: before_delete_size,
-              deleted: nil
-            )
+          service_response = expected_service_response(
+            status: status,
+            original_size: original_size,
+            before_truncate_size: before_truncate_size,
+            after_truncate_size: after_truncate_size,
+            before_delete_size: before_delete_size,
+            deleted: nil
+          )
 
-            expect(result).to eq(service_response)
-          end
+          expect(result).to eq(service_response)
         end
+      end
 
-        where(:feature_flag_enabled, :max_list_size, :delete_tags_service_status, :expected_status, :expected_truncated) do
-          false | 10 | :success | :success | false
-          false | 10 | :error   | :error   | false
-          false | 3  | :success | :success | false
-          false | 3  | :error   | :error   | false
-          false | 0  | :success | :success | false
-          false | 0  | :error   | :error   | false
-          true  | 10 | :success | :success | false
-          true  | 10 | :error   | :error   | false
-          true  | 3  | :success | :error   | true
-          true  | 3  | :error   | :error   | true
-          true  | 0  | :success | :success | false
-          true  | 0  | :error   | :error   | false
-        end
+      where(:feature_flag_enabled, :max_list_size, :delete_tags_service_status, :expected_status, :expected_truncated) do
+        false | 10 | :success | :success | false
+        false | 10 | :error   | :error   | false
+        false | 3  | :success | :success | false
+        false | 3  | :error   | :error   | false
+        false | 0  | :success | :success | false
+        false | 0  | :error   | :error   | false
+        true  | 10 | :success | :success | false
+        true  | 10 | :error   | :error   | false
+        true  | 3  | :success | :error   | true
+        true  | 3  | :error   | :error   | true
+        true  | 0  | :success | :success | false
+        true  | 0  | :error   | :error   | false
+      end
 
-        with_them do
-          before do
-            stub_feature_flags(container_registry_expiration_policies_throttling: feature_flag_enabled)
-            stub_application_setting(container_registry_cleanup_tags_service_max_list_size: max_list_size)
-            allow_next_instance_of(Projects::ContainerRepository::DeleteTagsService) do |service|
-              expect(service).to receive(:execute).and_return(status: delete_tags_service_status)
-            end
+      with_them do
+        before do
+          stub_feature_flags(container_registry_expiration_policies_throttling: feature_flag_enabled)
+          stub_application_setting(container_registry_cleanup_tags_service_max_list_size: max_list_size)
+          allow_next_instance_of(Projects::ContainerRepository::DeleteTagsService) do |service|
+            expect(service).to receive(:execute).and_return(status: delete_tags_service_status)
           end
+        end
 
-          original_size = 7
-          keep_n = 1
+        original_size = 7
+        keep_n = 1
 
-          it_behaves_like(
-            'returning the response',
-            status: params[:expected_status],
-            original_size: original_size,
-            before_truncate_size: original_size - keep_n,
-            after_truncate_size: params[:expected_truncated] ? params[:max_list_size] + keep_n : original_size - keep_n,
-            before_delete_size: params[:expected_truncated] ? params[:max_list_size] : original_size - keep_n - 1 # one tag is filtered out with older_than filter
-          )
-        end
+        it_behaves_like(
+          'returning the response',
+          status: params[:expected_status],
+          original_size: original_size,
+          before_truncate_size: original_size - keep_n,
+          after_truncate_size: params[:expected_truncated] ? params[:max_list_size] + keep_n : original_size - keep_n,
+          before_delete_size: params[:expected_truncated] ? params[:max_list_size] : original_size - keep_n - 1 # one tag is filtered out with older_than filter
+        )
       end
     end
 
-    context 'caching' do
+    context 'caching', :freeze_time do
       let(:params) do
         {
           'name_regex_delete' => '.*',
@@ -381,17 +379,12 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_
 
       before do
         expect_delete(%w(Bb Ba C), container_expiration_policy: true)
-        travel_to(Time.zone.local(2021, 9, 2, 12, 0, 0))
         # We froze time so we need to set the created_at stubs again
         stub_digest_config('sha256:configA', 1.hour.ago)
         stub_digest_config('sha256:configB', 5.days.ago)
         stub_digest_config('sha256:configC', 1.month.ago)
       end
 
-      after do
-        travel_back
-      end
-
       it 'caches the created_at values' do
         ::Gitlab::Redis::Cache.with do |redis|
           expect_mget(redis, tags_and_created_ats.keys)
@@ -450,32 +443,6 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_
         DateTime.rfc3339(date_time.rfc3339).rfc3339
       end
     end
-
-    context 'with container_registry_expiration_policies_caching enabled for the project' do
-      before do
-        stub_feature_flags(container_registry_expiration_policies_caching: project)
-      end
-
-      it_behaves_like 'reading and removing tags', caching_enabled: true
-    end
-
-    context 'with container_registry_expiration_policies_caching disabled' do
-      before do
-        stub_feature_flags(container_registry_expiration_policies_caching: false)
-      end
-
-      it_behaves_like 'reading and removing tags', caching_enabled: false
-    end
-
-    context 'with container_registry_expiration_policies_caching not enabled for the project' do
-      let_it_be(:another_project) { create(:project) }
-
-      before do
-        stub_feature_flags(container_registry_expiration_policies_caching: another_project)
-      end
-
-      it_behaves_like 'reading and removing tags', caching_enabled: false
-    end
   end
 
   private