Merge branch 'cleanup_policy_regex_defaults' into 'master'

Set defaults for cleanup policy regexes

See merge request gitlab-org/gitlab!44757

app/assets/javascripts/registry/settings/components/settings_form.vue

@@ -117,8 +117,9 @@ export default {
           const errorMessage = data?.updateContainerExpirationPolicy?.errors[0];
           if (errorMessage) {
             this.$toast.show(errorMessage, { type: 'error' });
+          } else {
+            this.$toast.show(UPDATE_SETTINGS_SUCCESS_MESSAGE, { type: 'success' });
           }
-          this.$toast.show(UPDATE_SETTINGS_SUCCESS_MESSAGE, { type: 'success' });
         })
         .catch(error => {
           this.setApiErrors(error);

app/assets/javascripts/registry/shared/constants.js

@@ -32,7 +32,7 @@ export const KEEP_N_LABEL = s__('ContainerRegistry|Number of tags to retain:');
 export const NAME_REGEX_LABEL = s__(
   'ContainerRegistry|Tags with names matching this regex pattern will %{italicStart}expire:%{italicEnd}',
 );
-export const NAME_REGEX_PLACEHOLDER = '.*';
+export const NAME_REGEX_PLACEHOLDER = '';
 export const NAME_REGEX_DESCRIPTION = s__(
   'ContainerRegistry|Wildcards such as %{codeStart}.*-test%{codeEnd} or %{codeStart}dev-.*%{codeEnd} are supported. To select all tags, use %{codeStart}.*%{codeEnd}',
 );

app/models/container_expiration_policy.rb

@@ -21,6 +21,7 @@ class ContainerExpirationPolicy < ApplicationRecord
   validates :cadence, presence: true, inclusion: { in: ->(_) { self.cadence_options.stringify_keys } }
   validates :older_than, inclusion: { in: ->(_) { self.older_than_options.stringify_keys } }, allow_nil: true
   validates :keep_n, inclusion: { in: ->(_) { self.keep_n_options.keys } }, allow_nil: true
+  validates :name_regex, presence: true, if: :enabled?
   validates :name_regex, untrusted_regexp: true, if: :enabled?
   validates :name_regex_keep, untrusted_regexp: true, if: :enabled?
 

changelogs/unreleased/cleanup_policy_regex_defaults.yml

+---
+title: Add default regexes and prevent blank regexes for container cleanup policies
+merge_request: 44757
+author:
+type: changed

db/migrate/20201008144854_set_regex_defaults_on_container_expiration_policies.rb

+# frozen_string_literal: true
+
+class SetRegexDefaultsOnContainerExpirationPolicies < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      change_column_default :container_expiration_policies, :name_regex, '.*'
+      change_column_default :container_expiration_policies, :enabled, false
+    end
+  end
+
+  def down
+    with_lock_retries do
+      change_column_default :container_expiration_policies, :name_regex, nil
+      change_column_default :container_expiration_policies, :enabled, true
+    end
+  end
+end

db/schema_migrations/20201008144854

+250785e18682cc10afb4f04546e5ff6dff9ab6c6673df84692c8221d6fe820ac
\ No newline at end of file

db/structure.sql

@@ -11235,11 +11235,11 @@ CREATE TABLE container_expiration_policies (
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
     next_run_at timestamp with time zone,
-    name_regex character varying(255),
+    name_regex character varying(255) DEFAULT '.*'::character varying,
     cadence character varying(12) DEFAULT '1d'::character varying NOT NULL,
     older_than character varying(12) DEFAULT '90d'::character varying,
     keep_n integer DEFAULT 10,
-    enabled boolean DEFAULT true NOT NULL,
+    enabled boolean DEFAULT false NOT NULL,
     name_regex_keep text,
     CONSTRAINT container_expiration_policies_name_regex_keep CHECK ((char_length(name_regex_keep) <= 255))
 );

doc/user/packages/container_registry/index.md

@@ -522,7 +522,7 @@ To create a cleanup policy in the UI:
    | **Expiration interval**                                                   | How long tags are exempt from being deleted.                                                                      |
    | **Expiration schedule**                                                   | How often the policy should run.                                                                                  |
    | **Number of tags to retain**                                              | How many tags to _always_ keep for each image.                                                                    |
-   | **Tags with names matching this regex pattern expire:**              | The regex pattern that determines which tags to remove. For all tags, use `.*`. See other [regex pattern examples](#regex-pattern-examples). |
+   | **Tags with names matching this regex pattern expire:**              | The regex pattern that determines which tags to remove. This value cannot be blank. For all tags, use `.*`. See other [regex pattern examples](#regex-pattern-examples). |
    | **Tags with names matching this regex pattern are preserved:**        | The regex pattern that determines which tags to preserve. The `latest` tag is always preserved. For all tags, use `.*`. See other [regex pattern examples](#regex-pattern-examples). |
 
 1. Click **Set cleanup policy**.
@@ -544,6 +544,8 @@ Here are examples of regex patterns you may want to use:
   .*
   ```
 
+  This is the default value for the expiration regex.
+
 - Match tags that start with `v`:
 
   ```plaintext

spec/features/projects/settings/registry_settings_spec.rb

@@ -15,6 +15,7 @@ RSpec.describe 'Project > Settings > CI/CD > Container registry tag expiration p
 
   before do
     project.update!(container_registry_enabled: container_registry_enabled_on_project)
+    project.container_expiration_policy.update!(enabled: true)
 
     sign_in(user)
     stub_container_registry_config(enabled: container_registry_enabled)

spec/frontend/registry/shared/components/__snapshots__/expiration_policy_fields_spec.js.snap

@@ -123,7 +123,7 @@ exports[`Expiration Policy Form renders 1`] = `
       disabled="true"
       id="expiration-policy-name-matching"
       noresize="true"
-      placeholder=".*"
+      placeholder=""
       trim=""
       value=""
     />

spec/graphql/mutations/container_expiration_policies/update_spec.rb

@@ -35,7 +35,7 @@ RSpec.describe Mutations::ContainerExpirationPolicies::Update do
 
         it_behaves_like 'not creating the container expiration policy'
 
-        it "doesn't update the cadence" do
+        it 'doesn\'t update the cadence' do
           expect { subject }
             .not_to change { container_expiration_policy.reload.cadence }
         end
@@ -47,6 +47,24 @@ RSpec.describe Mutations::ContainerExpirationPolicies::Update do
           )
         end
       end
+
+      context 'with blank regex' do
+        let_it_be(:params) { { project_path: project.full_path, name_regex: '', enabled: true } }
+
+        it_behaves_like 'not creating the container expiration policy'
+
+        it "doesn't update the cadence" do
+          expect { subject }
+            .not_to change { container_expiration_policy.reload.cadence }
+        end
+
+        it 'returns an error' do
+          expect(subject).to eq(
+            container_expiration_policy: nil,
+            errors: ['Name regex can\'t be blank']
+          )
+        end
+      end
     end
 
     RSpec.shared_examples 'denying access to container expiration policy' do

spec/lib/gitlab/usage_data_spec.rb

@@ -546,13 +546,13 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
       subject { described_class.data[:counts] }
 
       it 'gathers usage data' do
-        expect(subject[:projects_with_expiration_policy_enabled]).to eq 22
-        expect(subject[:projects_with_expiration_policy_disabled]).to eq 1
+        expect(subject[:projects_with_expiration_policy_enabled]).to eq 18
+        expect(subject[:projects_with_expiration_policy_disabled]).to eq 5
 
         expect(subject[:projects_with_expiration_policy_enabled_with_keep_n_unset]).to eq 1
         expect(subject[:projects_with_expiration_policy_enabled_with_keep_n_set_to_1]).to eq 1
         expect(subject[:projects_with_expiration_policy_enabled_with_keep_n_set_to_5]).to eq 1
-        expect(subject[:projects_with_expiration_policy_enabled_with_keep_n_set_to_10]).to eq 16
+        expect(subject[:projects_with_expiration_policy_enabled_with_keep_n_set_to_10]).to eq 12
         expect(subject[:projects_with_expiration_policy_enabled_with_keep_n_set_to_25]).to eq 1
         expect(subject[:projects_with_expiration_policy_enabled_with_keep_n_set_to_50]).to eq 1
 
@@ -560,9 +560,9 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
         expect(subject[:projects_with_expiration_policy_enabled_with_older_than_set_to_7d]).to eq 1
         expect(subject[:projects_with_expiration_policy_enabled_with_older_than_set_to_14d]).to eq 1
         expect(subject[:projects_with_expiration_policy_enabled_with_older_than_set_to_30d]).to eq 1
-        expect(subject[:projects_with_expiration_policy_enabled_with_older_than_set_to_90d]).to eq 18
+        expect(subject[:projects_with_expiration_policy_enabled_with_older_than_set_to_90d]).to eq 14
 
-        expect(subject[:projects_with_expiration_policy_enabled_with_cadence_set_to_1d]).to eq 18
+        expect(subject[:projects_with_expiration_policy_enabled_with_cadence_set_to_1d]).to eq 14
         expect(subject[:projects_with_expiration_policy_enabled_with_cadence_set_to_7d]).to eq 1
         expect(subject[:projects_with_expiration_policy_enabled_with_cadence_set_to_14d]).to eq 1
         expect(subject[:projects_with_expiration_policy_enabled_with_cadence_set_to_1month]).to eq 1

spec/models/container_expiration_policy_spec.rb

@@ -66,9 +66,15 @@ RSpec.describe ContainerExpirationPolicy, type: :model do
     end
 
     context 'with a set of regexps' do
+      let_it_be(:container_expiration_policy) { create(:container_expiration_policy) }
+
+      subject { container_expiration_policy }
+
       valid_regexps = %w[master .* v.+ v10.1.* (?:v.+|master|release)]
       invalid_regexps = ['[', '(?:v.+|master|release']
 
+      it { is_expected.to validate_presence_of(:name_regex) }
+
       valid_regexps.each do |valid_regexp|
         it { is_expected.to allow_value(valid_regexp).for(:name_regex) }
         it { is_expected.to allow_value(valid_regexp).for(:name_regex_keep) }
@@ -84,6 +90,8 @@ RSpec.describe ContainerExpirationPolicy, type: :model do
 
         subject { container_expiration_policy }
 
+        it { is_expected.not_to validate_presence_of(:name_regex) }
+
         valid_regexps.each do |valid_regexp|
           it { is_expected.to allow_value(valid_regexp).for(:name_regex) }
           it { is_expected.to allow_value(valid_regexp).for(:name_regex_keep) }

spec/requests/api/graphql/mutations/container_expiration_policy/update_spec.rb

@@ -73,6 +73,29 @@ RSpec.describe 'Updating the container expiration policy' do
     end
   end
 
+  RSpec.shared_examples 'rejecting blank name_regex when enabled' do
+    context "for blank name_regex" do
+      let(:params) do
+        {
+          project_path: project.full_path,
+          name_regex: '',
+          enabled: true
+        }
+      end
+
+      it_behaves_like 'returning response status', :success
+
+      it_behaves_like 'not creating the container expiration policy'
+
+      it 'returns an error' do
+        subject
+
+        expect(graphql_data['updateContainerExpirationPolicy']['errors'].size).to eq(1)
+        expect(graphql_data['updateContainerExpirationPolicy']['errors']).to include("Name regex can't be blank")
+      end
+    end
+  end
+
   RSpec.shared_examples 'accepting the mutation request updating the container expiration policy' do
     it_behaves_like 'updating the container expiration policy attributes', mode: :update, from: { cadence: '1d', keep_n: 10, older_than: '90d' }, to: { cadence: '3month', keep_n: 100, older_than: '14d' }
 
@@ -80,6 +103,7 @@ RSpec.describe 'Updating the container expiration policy' do
 
     it_behaves_like 'rejecting invalid regex for', :name_regex
     it_behaves_like 'rejecting invalid regex for', :name_regex_keep
+    it_behaves_like 'rejecting blank name_regex when enabled'
   end
 
   RSpec.shared_examples 'accepting the mutation request creating the container expiration policy' do
@@ -89,6 +113,7 @@ RSpec.describe 'Updating the container expiration policy' do
 
     it_behaves_like 'rejecting invalid regex for', :name_regex
     it_behaves_like 'rejecting invalid regex for', :name_regex_keep
+    it_behaves_like 'rejecting blank name_regex when enabled'
   end
 
   RSpec.shared_examples 'denying the mutation request' do

spec/requests/api/projects_spec.rb

@@ -2659,6 +2659,7 @@ RSpec.describe API::Projects do
         project_param = {
           container_expiration_policy_attributes: {
             cadence: '1month',
+            enabled: true,
             keep_n: 1,
             name_regex_keep: '['
           }

spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb

@@ -13,6 +13,10 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
   describe '#perform_work' do
     subject { worker.perform_work }
 
+    before do
+      policy.update_column(:enabled, true)
+    end
+
     RSpec.shared_examples 'handling all repository conditions' do
       it 'sends the repository for cleaning' do
         expect(ContainerExpirationPolicies::CleanupService)