Skip to content

Draft: PoC: Track all development feature flags in YAML (RUN AS-IF-FOSS)

Kamil Trzciński requested to merge track-feature-flags into master

Description of the proposal

I used the New static analysis check which seemed the most accurate.

For very long time I consider feature flag to be super handy mechanism, but also a ~"technical debt": #30228 (closed).

There's currently an ongoing effort to spread more knowledge how to use feature flags by Engineers at GitLab: gitlab-com/www-gitlab-com#7048 (closed)

This kind of builds on top of these two issues to have a better discoverability and manageability of feature flag.

This Proof of Concept introduces a config/feature-flags/my-feature.yaml and ee/config/feature-flags/my-feature.yaml that tries to describe each feature flag that we use in GitLab.

What this MR does?

This makes the following changes to process and code:

  1. Asks to create issue that tracks rollout and removal of FF
  2. Intentionally adds default_enabled to YAML to make it in the future a single source of truth whether the feature flag is enabled or disabled by default
  3. Adds bin/feature-flag script to easily create a new definition
  4. Changes how we stub "feature-flags" for specs, to ensure that normal Flipper semantics could be always used
  5. The lack of definition does not break production, it raises an exception in development/test environments only
  6. It expects that each feature flag is explicitly defined

The YAML syntax

name: rugged_commit_is_ancestor
introduced_by_url: # a URL to Merge Request introducing a feature flag
rollout_issue_url: # a URL to Issue discussing rollout of a feature flag
author: # likely not needed, as this can be easily taken from MR and Issue
group: 'group::apm' # a group that introduced a given feature flag
type: other # a type of feature flag: like FF for licensed feature, or just development/rollout type
default_enabled: false # a default state of feature flag if not defined

All the fields like _url are considered optional. For example, for licensed features we might not specify rollout_issue_url, but may introduced_by_url

The types

I currently proposed the following types of feature flags:

  1. licensed: Licensed feature, like ee/app/models/license.rb: contribution_analytics
  2. config: Feature flag used to configure features, like gitlab-com/gl-infra/scalability#327 (closed)
  3. development: Temporary feature flag used for feature development, flags that are considered to be temporary and are ~"technical debt" according to that definition: #30228 (closed)
  4. other: Other

The bin/feature-flags script

➜  gitlab-rails bin/feature-flag my-new-feature
>> Please specify the index for the category of your change:

1. licensed: Licensed feature
2. config: Feature flag used to configure features
3. development: Temporary feature flag used for feature development
4. other: Other

?> 3
>> Please specify the group introducing feature flag, like `group::apm`:

?> group::memory
>> Open this URL and fill the rest of details:
https://gitlab.com/gitlab-org/gitlab/-/issues/new?issue[title]=%5BFeature+flag%5D+Rollout+of+%60my-new-feature%60%60&

>> Paste URL here, or enter to skip:

?> https://gitlab.com/gitlab-org/gitlab/-/issues/12321
create config/feature-flags/my-new-feature.yml
---
name: my-new-feature
introduced_by_url:
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/12321
author:
group: group::memory
type: development
default_enabled: false

The idea of rollout

My vision for that is:

  1. Remove all usages of allow(Feature) and expect(Feature) from specs, ensure that we always use stub_feature_flags
  2. Create all definitions of current feature flags as YAML and change process how we use a new feature flag
  3. Ensure that default_enabled is coherent with YAML definition
  4. Remove , default_enabled: true from code, as this makes us to copy the same line over and over and possibly have a cases where we don't change all places that we we should define default_enabled

The intent

My intent of proposing that change is:

  1. See clearly what feature flags are used and can be easily tracked by issues and YAML definition
  2. Make it easy to quickly compare changes between two versions of GitLab, see what new feature flags were added/removed/changed
  3. Make it easy to track by PM the defaults of feature flags as used by on-premise installations
  4. Make it more descriptive on how and where we use feature flags
  5. Make it easy to know who owns the given FF and make it easier to collaborate on their usage and rollout
  6. Maybe in the future we also figure out some consistent naming for feature flags :)

Check-list

  • Make sure this MR enables a static analysis check rule for new usage but ignores current offenses
  • Mention this proposal in the relevant Slack channels (e.g. #development, #backend, #frontend)
  • If there is a choice to make between two potential styles, set up an emoji vote in the MR:
    • CHOICE_A: 🅰
    • CHOICE_B: 🅱
    • Vote yourself for both choices so that people know these are the choices
  • The MR doesn't have significant objections, and is getting a majority of 👍 vs 👎 (remember that we don't need to reach a consensus)
  • (If applicable) One style is getting a majority of vote (compared to the other choice)
  • (If applicable) Update the MR with the chosen style
  • Create a follow-up issue to fix the current offenses as a separate iteration: ISSUE_LINK
  • Follow the review process as usual
  • Once approved and merged by a maintainer, mention it again:
    • In the relevant Slack channels (e.g. #development, #backend, #frontend)
    • (Optional depending on the impact of the change) In the Engineering Week in Review

/cc @gitlab-org/maintainers/rails-backend

Edited by Craig Gomes

Merge request reports