Merge branch '18792-stop-writing-to-projects-container-registry-enabled' into 'master'

Write to ProjectFeature#container_registry_access_level

See merge request gitlab-org/gitlab!62663

app/models/concerns/project_features_compatibility.rb

@@ -90,6 +90,17 @@ module ProjectFeaturesCompatibility
     write_feature_attribute_string(:container_registry_access_level, value)
   end
 
+  # TODO: Remove this method after we drop support for project create/edit APIs to set the
+  # container_registry_enabled attribute. They can instead set the container_registry_access_level
+  # attribute.
+  def container_registry_enabled=(value)
+    write_feature_attribute_boolean(:container_registry_access_level, value)
+
+    # TODO: Remove this when we remove the projects.container_registry_enabled
+    # column. https://gitlab.com/gitlab-org/gitlab/-/issues/335425
+    super
+  end
+
   private
 
   def write_feature_attribute_boolean(field, value)

app/models/project.rb

@@ -76,7 +76,6 @@ class Project < ApplicationRecord
   default_value_for :packages_enabled, true
   default_value_for :archived, false
   default_value_for :resolve_outdated_diff_discussions, false
-  default_value_for :container_registry_enabled, gitlab_config_features.container_registry
   default_value_for(:repository_storage) do
     Repository.pick_storage_shard
   end
@@ -98,9 +97,6 @@ class Project < ApplicationRecord
 
   before_save :ensure_runners_token
 
-  # https://api.rubyonrails.org/v6.0.3.4/classes/ActiveRecord/AttributeMethods/Dirty.html#method-i-will_save_change_to_attribute-3F
-  before_update :set_container_registry_access_level, if: :will_save_change_to_container_registry_enabled?
-
   after_save :update_project_statistics, if: :saved_change_to_namespace_id?
 
   after_save :create_import_state, if: ->(project) { project.import? && project.import_state.nil? }
@@ -2668,20 +2664,6 @@ class Project < ApplicationRecord
 
   private
 
-  def set_container_registry_access_level
-    # changes_to_save = { 'container_registry_enabled' => [value_before_update, value_after_update] }
-    value = changes_to_save['container_registry_enabled'][1]
-
-    access_level =
-      if value
-        ProjectFeature::ENABLED
-      else
-        ProjectFeature::DISABLED
-      end
-
-    project_feature.update!(container_registry_access_level: access_level)
-  end
-
   def find_integration(integrations, name)
     integrations.find { _1.to_param == name }
   end

app/models/project_feature.rb

@@ -2,6 +2,7 @@
 
 class ProjectFeature < ApplicationRecord
   include Featurable
+  extend Gitlab::ConfigHelper
 
   # When updating this array, make sure to update rubocop/cop/gitlab/feature_available_usage.rb as well.
   FEATURES = %i[
@@ -48,8 +49,6 @@ class ProjectFeature < ApplicationRecord
     end
   end
 
-  before_create :set_container_registry_access_level
-
   # Default scopes force us to unscope here since a service may need to check
   # permissions for a project in pending_delete
   # http://stackoverflow.com/questions/1540645/how-to-disable-default-scope-for-a-belongs-to
@@ -80,6 +79,14 @@ class ProjectFeature < ApplicationRecord
     end
   end
 
+  default_value_for(:container_registry_access_level, allows_nil: false) do |feature|
+    if gitlab_config_features.container_registry
+      ENABLED
+    else
+      DISABLED
+    end
+  end
+
   def public_pages?
     return true unless Gitlab.config.pages.access_control
 
@@ -94,15 +101,6 @@ class ProjectFeature < ApplicationRecord
 
   private
 
-  def set_container_registry_access_level
-    self.container_registry_access_level =
-      if project&.read_attribute(:container_registry_enabled)
-        ENABLED
-      else
-        DISABLED
-      end
-  end
-
   # Validates builds and merge requests access level
   # which cannot be higher than repository access level
   def repository_children_level

ee/app/services/projects/setup_ci_cd.rb

@@ -13,7 +13,6 @@ module Projects
 
     def update_project
       project.update(
-        container_registry_enabled:          false,
         mirror:                              true,
         mirror_trigger_builds:               true,
         mirror_overwrites_diverged_branches: true,
@@ -24,10 +23,11 @@ module Projects
 
     def disable_project_features
       project.project_feature.update(
-        issues_access_level:         ProjectFeature::DISABLED,
-        merge_requests_access_level: ProjectFeature::DISABLED,
-        wiki_access_level:           ProjectFeature::DISABLED,
-        snippets_access_level:       ProjectFeature::DISABLED
+        issues_access_level:             ProjectFeature::DISABLED,
+        merge_requests_access_level:     ProjectFeature::DISABLED,
+        wiki_access_level:               ProjectFeature::DISABLED,
+        snippets_access_level:           ProjectFeature::DISABLED,
+        container_registry_access_level: ProjectFeature::DISABLED
       )
     end
   end

spec/factories/projects.rb

@@ -34,6 +34,7 @@ FactoryBot.define do
       end
       metrics_dashboard_access_level { ProjectFeature::PRIVATE }
       operations_access_level { ProjectFeature::ENABLED }
+      container_registry_access_level { ProjectFeature::ENABLED }
 
       # we can't assign the delegated `#ci_cd_settings` attributes directly, as the
       # `#ci_cd_settings` relation needs to be created first
@@ -70,6 +71,17 @@ FactoryBot.define do
       }
 
       project.build_project_feature(hash)
+
+      # This is not included in the `hash` above because the default_value_for in
+      # the ProjectFeature model overrides the value set by `build_project_feature` when
+      # evaluator.container_registry_access_level == ProjectFeature::DISABLED.
+      #
+      # This is because the default_value_for gem uses the <column>_changed? method
+      # to determine if the default value should be applied. For new records,
+      # <column>_changed? returns false if the value of the column is the same as
+      # the database default.
+      # See https://github.com/FooBarWidget/default_value_for/blob/release-3.4.0/lib/default_value_for.rb#L158.
+      project.project_feature.container_registry_access_level = evaluator.container_registry_access_level
     end
 
     after(:create) do |project, evaluator|
@@ -344,6 +356,9 @@ FactoryBot.define do
     trait(:analytics_enabled)           { analytics_access_level { ProjectFeature::ENABLED } }
     trait(:analytics_disabled)          { analytics_access_level { ProjectFeature::DISABLED } }
     trait(:analytics_private)           { analytics_access_level { ProjectFeature::PRIVATE } }
+    trait(:container_registry_enabled)  { container_registry_access_level { ProjectFeature::ENABLED } }
+    trait(:container_registry_disabled) { container_registry_access_level { ProjectFeature::DISABLED } }
+    trait(:container_registry_private)  { container_registry_access_level { ProjectFeature::PRIVATE } }
 
     trait :auto_devops do
       association :auto_devops, factory: :project_auto_devops

spec/models/container_repository_spec.rb

@@ -323,7 +323,7 @@ RSpec.describe ContainerRepository do
     context 'with a subgroup' do
       let_it_be(:test_group) { create(:group) }
       let_it_be(:another_project) { create(:project, path: 'test', group: test_group) }
-      let_it_be(:project3) { create(:project, path: 'test3', group: test_group, container_registry_enabled: false) }
+      let_it_be(:project3) { create(:project, :container_registry_disabled, path: 'test3', group: test_group) }
 
       let_it_be(:another_repository) do
         create(:container_repository, name: 'my_image', project: another_project)

spec/models/project_feature_spec.rb

@@ -189,27 +189,33 @@ RSpec.describe ProjectFeature do
   end
 
   describe 'container_registry_access_level' do
-    context 'when the project is created with container_registry_enabled false' do
-      it 'creates project with DISABLED container_registry_access_level' do
-        project = create(:project, container_registry_enabled: false)
+    context 'with default value' do
+      let(:project) { Project.new }
 
-        expect(project.project_feature.container_registry_access_level).to eq(described_class::DISABLED)
+      context 'when the default is false' do
+        it 'creates project_feature with `disabled` container_registry_access_level' do
+          stub_config_setting(default_projects_features: { container_registry: false })
+
+          expect(project.project_feature.container_registry_access_level).to eq(described_class::DISABLED)
+        end
       end
-    end
 
-    context 'when the project is created with container_registry_enabled true' do
-      it 'creates project with ENABLED container_registry_access_level' do
-        project = create(:project, container_registry_enabled: true)
+      context 'when the default is true' do
+        before do
+          stub_config_setting(default_projects_features: { container_registry: true })
+        end
 
-        expect(project.project_feature.container_registry_access_level).to eq(described_class::ENABLED)
+        it 'creates project_feature with `enabled` container_registry_access_level' do
+          expect(project.project_feature.container_registry_access_level).to eq(described_class::ENABLED)
+        end
       end
-    end
 
-    context 'when the project is created with container_registry_enabled nil' do
-      it 'creates project with DISABLED container_registry_access_level' do
-        project = create(:project, container_registry_enabled: nil)
+      context 'when the default is nil' do
+        it 'creates project_feature with `disabled` container_registry_access_level' do
+          stub_config_setting(default_projects_features: { container_registry: nil })
 
-        expect(project.project_feature.container_registry_access_level).to eq(described_class::DISABLED)
+          expect(project.project_feature.container_registry_access_level).to eq(described_class::DISABLED)
+        end
       end
     end
   end

spec/models/project_spec.rb

@@ -2407,7 +2407,7 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
-  describe '#set_container_registry_access_level' do
+  describe '#container_registry_enabled=' do
     let_it_be_with_reload(:project) { create(:project) }
 
     it 'updates project_feature', :aggregate_failures do