Make IS_GITLAB_EE consistent
What does this MR do?
Before these changes
- If
IS_GITLAB_EE=
is used, it fails to generate Webpack as it cannot be parsed as JSON, - If
IS_GITLAB_EE=1
is used, it fails on GitLab CE, - If
IS_GITLAB_EE=0
it does work correctly: a. and disables EE when run on EE, b. works correctly as well on CE, - 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
-
IS_GITLAB_EE=
orunset IS_GITLAB_EE
indicate to use a default: enable CE or EE depending on the presence ofee/app/models/license.rb
, -
IS_GITLAB_EE=0
makes to always unconditionally disable EE changes, -
IS_GITLAB_EE=1
makes to allow usage of EE changes if they are present by checking the existence ofee/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
-
Changelog entry -
Documentation created/updated or follow-up review issue created -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Performance and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers
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