From 4feeb5f3558bc5721142a6f5718c6cff490a8e11 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 13 Nov 2018 17:29:14 +0100 Subject: [PATCH] Refactor how a few ActiveRecord enums are defined In a few models we define ActiveRecord enums that are redefined in EE using the following pattern: enum :some_enum, { ... }.merge(EE_ENUM_VALUES) This particular approach is problematic to deal with, because it requires that we `prepend` and EE module _before_ defining the enum. This typically translates to the `prepend` being the first line in the model in EE, but this can easily lead to merge conflicts when developers add more `include` and/or `prepend` lines. As part of https://gitlab.com/gitlab-org/gitlab-ee/issues/8244 and https://gitlab.com/gitlab-org/gitlab-ee/issues/8241 we are moving `prepend` to the last line in a file, reducing the chances of running into merge conflicts. This poses a bit of a problem with the pattern above, because this pattern does not allow us to move the `prepend` further down a file. To resolve this problem, we simply move the Hash value of the enum to a separate class method. This method is defined in a separate module where necessary, allowing us to use it like so: enum :failure_reasons, ::SomeModelEnums.failure_reasons The method in turn is defined in a very straightforward manner: module SomeModelEnums def self.failure_reasons { ... } end end This makes it easy for EE to add values without requiring the `prepend` to be placed before the `enum` is defined. For more information, see the following issues and merge requests: * https://gitlab.com/gitlab-org/gitlab-ee/issues/8244 * https://gitlab.com/gitlab-org/gitlab-ee/issues/8241 * https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/8424 --- app/models/ci/pipeline.rb | 21 ++++--------- app/models/ci/pipeline_enums.rb | 30 +++++++++++++++++++ app/models/commit_status.rb | 15 ++-------- app/models/commit_status_enums.rb | 22 ++++++++++++++ app/models/user_callout.rb | 9 ++---- app/models/user_callout_enums.rb | 18 +++++++++++ app/presenters/ci/pipeline_presenter.rb | 13 ++++---- ee/app/models/ee/ci/pipeline_enums.rb | 23 ++++++++++++++ ee/app/models/ee/commit_status_enums.rb | 16 ++++++++++ ee/app/models/ee/user_callout_enums.rb | 16 ++++++++++ ee/app/presenters/ee/ci/pipeline_presenter.rb | 17 ++++++++--- 11 files changed, 158 insertions(+), 42 deletions(-) create mode 100644 app/models/ci/pipeline_enums.rb create mode 100644 app/models/commit_status_enums.rb create mode 100644 app/models/user_callout_enums.rb create mode 100644 ee/app/models/ee/ci/pipeline_enums.rb create mode 100644 ee/app/models/ee/commit_status_enums.rb create mode 100644 ee/app/models/ee/user_callout_enums.rb diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 3bb042158ebadf..e4128d47f8d2b9 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -69,17 +69,9 @@ class Pipeline < ActiveRecord::Base after_create :keep_around_commits, unless: :importing? - enum_with_nil source: { - unknown: nil, - push: 1, - web: 2, - trigger: 3, - schedule: 4, - api: 5, - external: 6, - pipeline: 7, - chat: 8 - } + # We use `Ci::PipelineEnums.sources` here so that EE can more easily extend + # this `Hash` with new values. + enum_with_nil source: ::Ci::PipelineEnums.sources enum_with_nil config_source: { unknown_source: nil, @@ -87,10 +79,9 @@ class Pipeline < ActiveRecord::Base auto_devops_source: 2 } - enum failure_reason: { - unknown_failure: 0, - config_error: 1 - }.merge(EE_FAILURE_REASONS) + # We use `Ci::PipelineEnums.failure_reasons` here so that EE can more easily + # extend this `Hash` with new values. + enum failure_reason: ::Ci::PipelineEnums.failure_reasons state_machine :status, initial: :created do event :enqueue do diff --git a/app/models/ci/pipeline_enums.rb b/app/models/ci/pipeline_enums.rb new file mode 100644 index 00000000000000..0cf07e8af8aa73 --- /dev/null +++ b/app/models/ci/pipeline_enums.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Ci + module PipelineEnums + # Returns the `Hash` to use for creating the `failure_reason` enum for + # `Ci::Pipeline`. + def self.failure_reasons + { + unknown_failure: 0, + config_error: 1 + } + end + + # Returns the `Hash` to use for creating the `sources` enum for + # `Ci::Pipeline`. + def self.sources + { + unknown: nil, + push: 1, + web: 2, + trigger: 3, + schedule: 4, + api: 5, + external: 6 + } + end + end +end + +Ci::PipelineEnums.prepend(EE::Ci::PipelineEnums) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 514bf2fa608f08..0a0d91907adc44 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -44,18 +44,9 @@ class CommitStatus < ActiveRecord::Base scope :retried_ordered, -> { retried.ordered.includes(project: :namespace) } scope :after_stage, -> (index) { where('stage_idx > ?', index) } - enum_with_nil failure_reason: { - unknown_failure: nil, - script_failure: 1, - api_failure: 2, - stuck_or_timeout_failure: 3, - runner_system_failure: 4, - missing_dependency_failure: 5, - runner_unsupported: 6, - stale_schedule: 7, - job_execution_timeout: 8, - archived_failure: 9 - }.merge(EE_FAILURE_REASONS) + # We use `CommitStatusEnums.failure_reasons` here so that EE can more easily + # extend this `Hash` with new values. + enum_with_nil failure_reason: ::CommitStatusEnums.failure_reasons ## # We still create some CommitStatuses outside of CreatePipelineService. diff --git a/app/models/commit_status_enums.rb b/app/models/commit_status_enums.rb new file mode 100644 index 00000000000000..bbc5fdd7084439 --- /dev/null +++ b/app/models/commit_status_enums.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module CommitStatusEnums + # Returns the Hash to use for creating the `failure_reason` enum for + # `CommitStatus`. + def self.failure_reasons + { + unknown_failure: nil, + script_failure: 1, + api_failure: 2, + stuck_or_timeout_failure: 3, + runner_system_failure: 4, + missing_dependency_failure: 5, + runner_unsupported: 6, + stale_schedule: 7, + job_execution_timeout: 8, + archived_failure: 9 + } + end +end + +CommitStatusEnums.prepend(EE::CommitStatusEnums) diff --git a/app/models/user_callout.rb b/app/models/user_callout.rb index 2c0e8659fc19f7..76e7bc06b4ecec 100644 --- a/app/models/user_callout.rb +++ b/app/models/user_callout.rb @@ -3,12 +3,9 @@ class UserCallout < ActiveRecord::Base belongs_to :user - enum feature_name: { - gke_cluster_integration: 1, - gcp_signup_offer: 2, - cluster_security_warning: 3, - gold_trial: 4 - } + # We use `UserCalloutEnums.feature_names` here so that EE can more easily + # extend this `Hash` with new values. + enum feature_name: ::UserCalloutEnums.feature_names validates :user, presence: true validates :feature_name, diff --git a/app/models/user_callout_enums.rb b/app/models/user_callout_enums.rb new file mode 100644 index 00000000000000..6dfffe9c093687 --- /dev/null +++ b/app/models/user_callout_enums.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module UserCalloutEnums + # Returns the `Hash` to use for the `feature_name` enum in the `UserCallout` + # model. + # + # This method is separate from the `UserCallout` model so that it can be + # extended by EE. + def self.feature_names + { + gke_cluster_integration: 1, + gcp_signup_offer: 2, + cluster_security_warning: 3 + } + end +end + +UserCalloutEnums.prepend(EE::UserCalloutEnums) diff --git a/app/presenters/ci/pipeline_presenter.rb b/app/presenters/ci/pipeline_presenter.rb index d06d945d8560ef..c160f5d71a4707 100644 --- a/app/presenters/ci/pipeline_presenter.rb +++ b/app/presenters/ci/pipeline_presenter.rb @@ -2,12 +2,13 @@ module Ci class PipelinePresenter < Gitlab::View::Presenter::Delegated - prepend ::EE::Ci::PipelinePresenter include Gitlab::Utils::StrongMemoize - FAILURE_REASONS = { - config_error: 'CI/CD YAML configuration error!' - }.merge(EE_FAILURE_REASONS) + # We use a class method here instead of a constant, allowing EE to redefine + # the returned `Hash` more easily. + def self.failure_reasons + { config_error: 'CI/CD YAML configuration error!' } + end presents :pipeline @@ -22,7 +23,7 @@ def failed_builds def failure_reason return unless pipeline.failure_reason? - FAILURE_REASONS[pipeline.failure_reason.to_sym] || + self.class.failure_reasons[pipeline.failure_reason.to_sym] || pipeline.failure_reason end @@ -33,3 +34,5 @@ def status_title end end end + +Ci::PipelinePresenter.prepend(EE::Ci::PipelinePresenter) diff --git a/ee/app/models/ee/ci/pipeline_enums.rb b/ee/app/models/ee/ci/pipeline_enums.rb new file mode 100644 index 00000000000000..7cd5d5aaee635c --- /dev/null +++ b/ee/app/models/ee/ci/pipeline_enums.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module EE + module Ci + module PipelineEnums + extend ActiveSupport::Concern + + class_methods do + extend ::Gitlab::Utils::Override + + override :failure_reasons + def failure_reasons + super.merge(activity_limit_exceeded: 20, size_limit_exceeded: 21) + end + + override :sources + def sources + super.merge(pipeline: 7, chat: 8) + end + end + end + end +end diff --git a/ee/app/models/ee/commit_status_enums.rb b/ee/app/models/ee/commit_status_enums.rb new file mode 100644 index 00000000000000..dfd386d38c7e21 --- /dev/null +++ b/ee/app/models/ee/commit_status_enums.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module CommitStatusEnums + extend ActiveSupport::Concern + + class_methods do + extend ::Gitlab::Utils::Override + + override :failure_reasons + def failure_reasons + super.merge(protected_environment_failure: 1_000) + end + end + end +end diff --git a/ee/app/models/ee/user_callout_enums.rb b/ee/app/models/ee/user_callout_enums.rb new file mode 100644 index 00000000000000..571f52a33f329e --- /dev/null +++ b/ee/app/models/ee/user_callout_enums.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module UserCalloutEnums + extend ActiveSupport::Concern + + class_methods do + extend ::Gitlab::Utils::Override + + override :feature_names + def feature_names + super.merge(cluster_security_warning: 3, gold_trial: 4) + end + end + end +end diff --git a/ee/app/presenters/ee/ci/pipeline_presenter.rb b/ee/app/presenters/ee/ci/pipeline_presenter.rb index c79fbb2e1a3699..63f931a1a32fee 100644 --- a/ee/app/presenters/ee/ci/pipeline_presenter.rb +++ b/ee/app/presenters/ee/ci/pipeline_presenter.rb @@ -1,10 +1,19 @@ module EE module Ci module PipelinePresenter - EE_FAILURE_REASONS = { - activity_limit_exceeded: 'Pipeline activity limit exceeded!', - size_limit_exceeded: 'Pipeline size limit exceeded!' - }.freeze + extend ActiveSupport::Concern + + class_methods do + extend ::Gitlab::Utils::Override + + override :failure_reasons + def failure_reasons + super.merge( + activity_limit_exceeded: 'Pipeline activity limit exceeded!', + size_limit_exceeded: 'Pipeline size limit exceeded!' + ) + end + end def expose_security_dashboard? any_report_artifact_for_type(:sast) || -- GitLab