Skip to content

RUN AS-IF-FOSS: Resolve "Find better place for app/models/*enum.rb modules"

Vitali Tatarintev requested to merge 202207-move-enum-modules-to-concerns-take-2 into master

What does this MR do?

Implements #202207 (closed)

Second attempt, because the first was reverted !38061 (merged) because it wasn't ran as-if-foss https://docs.gitlab.com/ee/development/pipelines.html#as-if-foss-jobs

Moves:

  • UserCalloutEnums to Enums::UserCallout
  • InternalIdEnums to Enums::InternalId
  • CommitStatusEnums to Enums::CommitStatus
  • PrometheusMetricEnums to Enums::PrometheusMetric
  • Ci::PipelineEnums to Enums::Ci::Pipeline

In a nutshell it moves modules in the models directory Enums namespace without changes.

There is also module with the similar name Gitlab::DatabaseImporters::CommonMetrics::PrometheusMetricEnums, but I'm not sure if that module falls into this category, because it's in lib directory and not in the models.

Modules vs Concerns

I've tried to extract them into concerns at first, as suggested in the parent issue #202207 (comment 283496052) but that creates a set of difficulties. Rails concerns with defined class_methods don't work well with prepend_if_ee. We have a couple of comments about that here and there.

Even using Module::ClassMethods.prepend_if_ee doesn't help in some cases. For example, it still doesn't extend a class method when it's called directly with ConcernModule.class_method.

Yet another difficulty is that concerns don't work well with override #23932 (closed)

Even replacing...

class_methods do
  extend ::Gitlab::Utils::Override

  override :method
  def method
    super.merge(ee_key: 1)
  end
end

... with ...

class_methods do
  def method
    {
      **super,
      ee_key: 1
    }
  end
end

... didn't help to fix all the scenarios.

Based on that, I would suggest keeping CE enums as modules (not concerns), that makes the codebase cleaner.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • ~~Label as security and @ mention @gitlab-com/gl-security/appsec~~
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Merge request reports