Skip to content

Make IS_GITLAB_EE consistent

Kamil Trzciński requested to merge make-is-gitlab-ee-consistent into master

What does this MR do?

Before these changes

  1. If IS_GITLAB_EE= is used, it fails to generate Webpack as it cannot be parsed as JSON,
  2. If IS_GITLAB_EE=1 is used, it fails on GitLab CE,
  3. If IS_GITLAB_EE=0 it does work correctly: a. and disables EE when run on EE, b. works correctly as well on CE,
  4. If IS_GITLAB_EE=1 is used on GitLab EE it effectively required to run on EE changes.

This changes to:

Make IS_GITLAB_EE= to actually present the intent to disable or enable EE sources, instead of requiring it, and makes to ensure that we have consistent in behaviour between backend and frontend.

Summary

  1. IS_GITLAB_EE= or unset IS_GITLAB_EE indicate to use a default: enable CE or EE depending on the presence of ee/app/models/license.rb,
  2. IS_GITLAB_EE=0 makes to always unconditionally disable EE changes,
  3. IS_GITLAB_EE=1 makes to allow usage of EE changes if they are present by checking the existence of ee/app/models/license.rb.

Additional consideration about naming

Now, looking at semantics of variable name the IS_GITLAB_EE seems to indicate the question. Which seems to be wrongly stated as it should rather indicate the intent not the outcome of intent which seems to be IS_GITLAB_EE.

Since, user has the intent to use or not use EE (effectively we want to not use EE in some circumstances) it would be better to consider naming that different or maybe even twisting the behaviour:

  • USE_GITLAB_EE: it is not a question as originally, but rather the intent of the user,
  • DISABLE_EE/DISABLE_GITLAB_EE: to indicate that you want to disable EE portion of the code base, that otherwise would be enabled

I personally would prefer to DISABLE_EE as it is much cleaner about the intent and the outcome without having to consider the side effects. As for everything that is DISABLE_EE != true that is not true, it means that you want to use EE changes if their are present, and I consider this to be default behaviour that people want to follow, and much easier to understand than usage of IS_GITLAB_EE where effectively it has three state: 1. missing, 2. set to false, 3. set to true.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Performance 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
Edited by Grzegorz Bizon

Merge request reports