Refactor: Remove delegating ProjectCiCdSettings to Project
What?
Refactor to remove delegation of ci_cd_settings through the project object.
Why?
It forces us into two sources of truth for defaults.
We must account for the quirks of predicate methods that no longer always return a boolean. This could cause buggy behavior or cause us to miss bugs.
It has the CI domain logic code living alongside logic from many other domains in the project model with no clear boundaries between the domains.
It causes the project model to grow and become a god object where logic from every domain lives.
Implementation
An incomplete example of removing 1 setting. Implementation of Project::CiCdSettings::UpdateService
not included.
diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb
index b330aacf3e95..c8898db83f29 100644
--- a/app/controllers/projects/settings/ci_cd_controller.rb
+++ b/app/controllers/projects/settings/ci_cd_controller.rb
@@ -31,7 +31,7 @@ def show
end
def update
- Projects::UpdateService.new(project, current_user, update_params).tap do |service|
+ Project::CiCdSettings::UpdateService.new(project, current_user, update_params).tap do |service|
result = service.execute
if result[:status] == :success
flash[:toast] = _("Pipelines settings for '%{project_name}' were successfully updated.") % { project_name: @project.name }
diff --git a/app/models/project.rb b/app/models/project.rb
index 52bf0a3738b2..9df9d5e3e1ac 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -491,7 +491,6 @@ def self.integration_association_name(name)
delegate :external_dashboard_url, to: :metrics_setting, allow_nil: true, prefix: true
delegate :dashboard_timezone, to: :metrics_setting, allow_nil: true, prefix: true
delegate :default_git_depth, :default_git_depth=, to: :ci_cd_settings, prefix: :ci, allow_nil: true
- delegate :forward_deployment_enabled, :forward_deployment_enabled=, to: :ci_cd_settings, prefix: :ci, allow_nil: true
delegate :job_token_scope_enabled, :job_token_scope_enabled=, to: :ci_cd_settings, prefix: :ci_outbound, allow_nil: true
delegate :inbound_job_token_scope_enabled, :inbound_job_token_scope_enabled=, to: :ci_cd_settings, prefix: :ci, allow_nil: true
delegate :keep_latest_artifact, :keep_latest_artifact=, to: :ci_cd_settings, allow_nil: true
@@ -2951,12 +2950,6 @@ def activity_path
Gitlab::Routing.url_helpers.activity_project_path(self)
end
- def ci_forward_deployment_enabled?
- return false unless ci_cd_settings
-
- ci_cd_settings.forward_deployment_enabled?
- end
-
def ci_allow_fork_pipelines_to_run_in_parent_project?
return false unless ci_cd_settings
diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb
index dcd1072612ab..df08b4c3cb91 100644
--- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb
+++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb
@@ -325,7 +325,7 @@ def show
subject
project.reload
- expect(project.ci_forward_deployment_enabled).to eq(false)
+ expect(project.ci_cd_settings.forward_deployment_enabled).to eq(false)
end
end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 097cd80903f4..170711010d37 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1110,7 +1110,6 @@
{
'group_runners_enabled' => '',
'default_git_depth' => 'ci_',
- 'forward_deployment_enabled' => 'ci_',
'keep_latest_artifact' => '',
'restrict_user_defined_variables' => '',
'runner_token_expiration_interval' => '',
@@ -1132,12 +1131,6 @@
end
end
- describe '#ci_forward_deployment_enabled?' do
- it_behaves_like 'a ci_cd_settings predicate method', prefix: 'ci_' do
- let(:delegated_method) { :forward_deployment_enabled? }
- end
- end
-
describe '#ci_allow_fork_pipelines_to_run_in_parent_project?' do
it_behaves_like 'a ci_cd_settings predicate method', prefix: 'ci_' do
let(:delegated_method) { :allow_fork_pipelines_to_run_in_parent_project? }
TODO: apply labels making it good for contributors.
Edited by Allison Browne