Skip to content

Have Danger warn of Feature Flag usage

Michelle Gill requested to merge danger-ff into master

What does this MR do?

This is one suggestion from gitlab-com/www-gitlab-com#6196 (closed) in order to improve the visibility of Feature Flags.

This MR will have Danger Bot recommend that a feature flag label be added to the MR if it meets certain criteria (added, modified, or removed file diffs contain the word Feature - case matched)

Screen_Shot_2020-02-27_at_2.58.55_PM

The intent is to catch code that surrounds subjects such as "Feature.enabled(foo)."

How to test

  1. Make a change to a file (Feature change or otherwise, depending on what you're testing) and commit it locally
  2. Run bin/rake danger_local to see what this change will output
  3. Blow that commit away because you should not use it

Pros

Our documentation already states that we should have the feature flag label on MRs that meet this criteria, so this rule will just help enforce that. The other pro is that EMs and others can look at the MRs associated with an issue to determine whether that work potentially went out behind a feature flag, without reviewing the code.

Cons

As you can see with this very MR, I am not making any changes to a feature flag, or working behind a feature flag, but there is a Danger warning anyway because my diff mentions Feature. This would also be true if I made an unrelated change below a line that contained Feature, since that line would still be part of a diff.

Similarly, it will also not catch changes that you make behind a feature flag when those changes don't "touch" any Feature work.

Next Steps

I think there are 2 big questions with this MR...

  1. Will it work for what we want it to?
  2. Is this solution needed?

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
Edited by Michelle Gill

Merge request reports