Have Danger warn of Feature Flag usage
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)
The intent is to catch code that surrounds subjects such as "Feature.enabled(foo)."
How to test
- Make a change to a file (Feature change or otherwise, depending on what you're testing) and commit it locally
- Run
bin/rake danger_local
to see what this change will output - 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...
- Will it work for what we want it to?
- Is this solution needed?
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability 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 -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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