## Problem We have the ability to use `Gitlab.com?` (and friends such as `Gitlab.dev_env_org_or_com?`) as code. Outside of narrow concerns, this should be discouraged in favor of data-driven approaches. ### Why is this a problem ? 1. Affects GitLab JH, and any other future SaaS instances 1. "Contagious" technical debt 1. Proliferation of conditional. When a related place uses `if GitLab.com?`, another piece of code, e.g. view will also need `if GitLab.com?`. \[1\] 1. Testing complexity - https://gitlab.com/groups/gitlab-org/-/epics/1969#note_226189771) 1. Makes this hard to test locally 1. Missed opportunity to provide similar features to Self-managed 1. Open source stewardship. Probably in conflict with our [open-source stewardship](https://about.gitlab.com/company/stewardship/) responsibilities \[1\] See also contagion metric from https://technology.riotgames.com/news/taxonomy-tech-debt article ### Current Scenarios where we check `.com`/SaaS 1. Subscriptions/Billing specific items - [JH doesn't want to apply Free User Cap](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/89474) - scenario type: `strategic` - current solution: `prepend_mod` in Core product and then overriding in `jh` - [General Billing items](https://gitlab.com/gitlab-org/gitlab/blob/df6065f3fe7155bd7f8ce3a976e326e8e40951b0/ee/lib/ee/sidebars/groups/menus/settings_menu.rb#L127-127) - scenario type: `feature`(?) - current solution: use `Gitlab::CurrentSettings.should_check_namespace_plan?` which is a combination of an application setting and checking this `Rails.env.test? || ::Gitlab.org_or_com?` based on `url` value and similar. 1. [Experimentation](https://gitlab.com/gitlab-org/gitlab/blob/df6065f3fe7155bd7f8ce3a976e326e8e40951b0/lib/gitlab/experiment/rollout/feature.rb#L16-16) - scenario type: `strategic` - current solution: use `Gitlab.com?` which validates the `url` or the environment variable `GITLAB_SIMULATE_SAAS` in `development` 1. Adding features only in `jh` - [Add phone number to registration form](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86844) - scenario type: `feature` - current solution: add an application setting for this feature. ## Possible solutions to `jh` and our own code delivery/organization of `.com` code 1. Implement a `feature_available?` concept described in https://gitlab.com/groups/gitlab-org/-/epics/7374#note_990484289 - pros: - scaleable - provides context as to why something is guarding for a specific instance. - makes it hard to disable unintentionally like is possible with an application setting - cons: May inadvertently make targeting code for only one instance easier. 1. Use `prepend_mod` as seen in the [Free User Cap](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/89474) above. - pros: allows override - cons: - implementation is fragile for `jh` in the scenario we change the code in the core product and their prepended code may be broken if we change the item they are overriding. - `jh` is always playing catchup to remove/analyze new features and then either accept or override them. Automatic `Opt-in`, which is likely unavoidable. 1. Use and enum instance and described above in https://gitlab.com/groups/gitlab-org/-/epics/7374#note_985157601 - pros: - shifts current logic overall from `url` based with override on `jh` and others(?) of the `Gitlab.com?` method if they want `.com` features. - makes the `.com` items `Opt-in` instead of `Opt-out`. Ideally in this case `jh` would add `instance_jh?` to items they want to accept as well from `.com` and we'd no longer be blind to `jh` as much in the core product. I'm not sure this is what we'd want, but at least we might not break them as easily as we could with the `prepend_mod` approach. - cons: - `jh` no longer gets features for purely `.com` by default if it is a new feature/area in `.com`. Could be a `pro` as well here as they wouldn't get `free user cap` by default as we would likely have this behind a `instance_com?` check. - Cases where `jh` or other future instances want specific items but not others from `.com`, so they'd have to put their instance check(`instance_jh?`) in a lot of places in the core product and that may not be what we want. However, this fits the `Opt-in` concept of this setting. 1. Create a new application setting for each `.com`/instance specific feature - see [Add phone number to registration](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86844) - pros: - Allows each instance to branch its own way. (this could also be a `con`) - Could be used as either `Opt-in` or `Opt-out` by default depending on the default setting. Likely assume `Opt-out` and default the setting `on`. - May allow current development practices overall to remain unchanged(? Not sure about this one) - cons: - Does this scale? We'd be promoting adding many new application settings and perhaps adding another layer of coordination on GitLab side with `jh`. Sometimes they'd only exist if instances of `jh` didn't want them or wanted to branch in a different way. <details><summary>previous proposal for more detail</summary> Use application settings. This enables granular settings so that each SaaS instance can switch things according to their need. For example, we have the `should_check_namespace_plan` application setting if the SaaS instance wants to check if the instance should check namespace-based plan checking (as opposed to instance-based plan checking). [ Note: It is not a simple matter of shifting from `if GitLab.com?` to `should_check_namespace_plan?`. The analogy is that we don't use a single feature flag for all features either ] We can consider having [SaaS-only application settings](https://gitlab.com/gitlab-org/gitlab/-/issues/225101#note_790567065) as well </details> 5. Separate `.com` code into a `/com` layer - pros: - Allows us to layer `/com` code like we do with `/ee` and get all the inherent benefits there. - Allows easier(? opinion) understanding when code is for `SaaS` and not other instances. - cons: - With this, we'd assume instances like `jh` would or wouldn't get the `/com` layer - If they did get the `/com` layer, it doesn't solve the `jh` issue of wanting to `Opt-out`/branch from `.com` code. They would still look to `Opt-out` at times and want to use `prepend_mod` or other strategies. - If they didn't get the `/com` layer then they wouldn't get the billing or subscription code at times and that causes a whole other set of issues. 6. Do nothing - pros: Nothing to do. But perhaps we should push more towards not having this issue in other ways? Encourage other instances to not `Opt-out` or something else? - cons: Doesn't solve our issue and still promotes use of `.com?` in the codebase. ## Iterations 1. Convert an existing call to an application setting 1. Write Rubocop to ban `Gitlab.com?` (and friends) from being called. ### Detail #### Numbers The number of calls for `GitLab.com?` is growing steadily | Version | Total | |---------|-------| | master | 196 | | 13-12-stable-ee | 178 | | 13-11-stable-ee | 158 | | 13-10-stable-ee | 126 | | 13-9-stable-ee | 119 | | 13-8-stable-ee | 113 | | 13-7-stable-ee | 112 | | 13-6-stable-ee | 108 | | 13-5-stable-ee | 101 | | 13-4-stable-ee | 96 | | 13-3-stable-ee | 88 | | 13-2-stable-ee | 83 | | 13-1-stable-ee | 80 | | 13-0-stable-ee | 76 | <details> <summary>Detail for each method</summary> | Version | Gitlab.canary? | Gitlab.com do | Gitlab.com? | Gitlab.com_and_canary? | Gitlab.com_but_not_canary? | Gitlab.dev_env_or_com? | Gitlab.dev_env_org_or_com? | Gitlab.dev_or_test_env? | Gitlab.extensions | Gitlab.gl_subdomain? | Gitlab.jh do | Gitlab.jh? | Gitlab.org? | Gitlab.staging? | |---------|----------------|---------------|-------------|------------------------|----------------------------|------------------------|----------------------------|-------------------------|-------------------|----------------------|--------------|------------|-------------|-----------------| | master | 1 | 1 | 130 | 1 | 2 | 26 | 5 | 16 | 1 | 0 | 2 | 4 | 1 | 6 | | 13-12-stable-ee | 1 | 1 | 117 | 1 | 2 | 25 | 5 | 13 | 1 | 0 | 2 | 4 | 1 | 5 | | 13-11-stable-ee | 1 | 1 | 110 | 1 | 2 | 19 | 5 | 7 | 0 | 0 | 2 | 4 | 1 | 5 | | 13-10-stable-ee | 1 | 1 | 92 | 1 | 2 | 16 | 5 | 4 | 0 | 0 | 0 | 0 | 1 | 3 | | 13-9-stable-ee | 1 | 1 | 88 | 2 | 2 | 13 | 5 | 3 | 0 | 0 | 0 | 0 | 0 | 4 | | 13-8-stable-ee | 1 | 0 | 87 | 1 | 2 | 11 | 5 | 3 | 0 | 0 | 0 | 0 | 0 | 3 | | 13-7-stable-ee | 1 | 0 | 86 | 1 | 2 | 11 | 5 | 4 | 0 | 0 | 0 | 0 | 0 | 2 | | 13-6-stable-ee | 1 | 0 | 81 | 1 | 2 | 12 | 4 | 5 | 0 | 0 | 0 | 0 | 0 | 2 | | 13-5-stable-ee | 1 | 0 | 74 | 1 | 2 | 13 | 4 | 4 | 0 | 0 | 0 | 0 | 0 | 2 | | 13-4-stable-ee | 1 | 0 | 72 | 1 | 2 | 12 | 4 | 2 | 0 | 0 | 0 | 0 | 0 | 2 | | 13-3-stable-ee | 1 | 0 | 65 | 1 | 2 | 11 | 4 | 2 | 0 | 0 | 0 | 0 | 0 | 2 | | 13-2-stable-ee | 1 | 0 | 63 | 1 | 2 | 8 | 4 | 2 | 0 | 0 | 0 | 0 | 0 | 2 | | 13-1-stable-ee | 1 | 0 | 62 | 1 | 2 | 6 | 4 | 2 | 0 | 0 | 0 | 0 | 0 | 2 | | 13-0-stable-ee | 1 | 0 | 62 | 1 | 2 | 6 | 4 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | </details> Generated with [count.rb](/uploads/7c1b26772db2e91b87a91be8fbf08b41/count.rb) #### So what is acceptable ? * Consider using application setting, like `should_check_namespace_plan?` * Or, consider [plan limits](https://docs.gitlab.com/ee/development/application_limits.html#development) * For ~"Enterprise Edition", also consider using License `feature_available?` * `require_foo_feature?` - finer grained checks - https://gitlab.com/gitlab-org/gitlab/-/issues/225101#note_602424460 * For strictly SaaS only concerns, we can create a `saas_only_application_settings` table. This way concerns like CustomersDot can be effectively scoped to only the SaaS instance that requires it. ## See also * https://gitlab.slack.com/archives/C02PF508L/p1592438620302400 (internal Slack thread) * https://gitlab.com/groups/gitlab-org/-/epics/1969#note_225793532 * https://docs.gitlab.com/ee/development/application_limits.html#development