Ignore `default_enabled` value in `Feature.enabled?` [RUN ALL RSPEC] [RUN AS-IF-FOSS]
What does this MR do?
Contributes to #271275 (closed)
Ignores default_enabled argument in Feature.enabled?. Takes default_enabled value from the feature flag definition file instead.
That should ensure that everything works as expected without default_enabled argument.
As a side note, default_enabled argument value is always in sync with the default_enabled value in the YAML definition file.
We cannot even set the argument value to be different from the one specified in the YAML file.
To make the MR smaller and enable ability to remove default_enabled from Feature.enabled? calls in chunks, I think we can make the following changes separately.
The only part I'm not sure about is either should we update the docs now to mention that the argument it ignored, or leave it till we change the docs after removing default_enabled argument completely.
Follow-up changes
-
Remove default_enabledfromFeature.enabled?calls -
Remove default_enabledargument fromFeature.enabled? -
Remove default_enabledfromFeature::Definition.valid_usage! -
Updated docs: Remove mentions of default_enabled -
Add a changelog entry (?)
Screenshots (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
- [-] I have included a changelog entry.
- [-] I have not included a changelog entry because _____.
- [-] 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
Merge request reports
Activity
1 Warning 
For the following files, a review from the Data team and Product Intelligence team is recommended
Please check the product intelligence guide and reach out to @gitlab-org/growth/product-intelligence/engineers group for a review.spec/requests/api/usage_data_spec.rb
2 Messages 
CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, run the following:
bin/changelog -m 56746 "Ignore \`default_enabled\` value in \`Feature.enabled?\` [RUN ALL RSPEC] [RUN AS-IF-FOSS]"If you want to create a changelog entry for GitLab EE, run the following instead:
bin/changelog --ee -m 56746 "Ignore \`default_enabled\` value in \`Feature.enabled?\` [RUN ALL RSPEC] [RUN AS-IF-FOSS]"If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
You're adding or removing a feature flag, your MR title needs to include [RUN ALL RSPEC] [RUN AS-IF-FOSS](we may have updated it automatically for you and started a new MR pipeline) to ensure everything is covered.Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
Category Reviewer Maintainer backend Vasilii Iakliushin ( @vyaklushin) (UTC+1, same timezone as@ck3g)Stan Hu ( @stanhu) (UTC-7, 8 hours behind@ck3g)If needed, you can retry the
danger-reviewjob that generated this comment.Generated by
Edited by 🤖 GitLab Bot 🤖added 540 commits
-
edee19f8...fbfeff74 - 537 commits from branch
master - dad67c6d - Ignore default_enabled arg in Feature.enabled?
- 74f38d72 - Add missing feature flag definition files
- 77770dd3 - Adjust feature flag related tests
Toggle commit list-
edee19f8...fbfeff74 - 537 commits from branch
added product intelligence product intelligencereview pending labels
Resolved by Mayra CabreraIt looks like I need to set groupmemory and groupsource code to fix the following danger errors:
groupis set to groupmemory, which does not match ~"group::monitor" set on the MR!groupis set to groupsource code, which does not match ~"group::monitor" set on the MR!Edited by Vitali Tatarintev- Last reply by Vitali Tatarintev
added groupsource code label and removed grouprespond label
added grouprespond label and removed groupsource code label
added typemaintenance label
added typefeature label
changed milestone to %13.11
mentioned in merge request !57215 (merged)
mentioned in merge request !57217 (merged)
added 468 commits
-
bb78e5c1...5b8dcbe6 - 465 commits from branch
master - 92d09451 - Ignore default_enabled arg in Feature.enabled?
- fba6d060 - Add missing feature flag definition files
- 011417d6 - Adjust feature flag related tests
Toggle commit list-
bb78e5c1...5b8dcbe6 - 465 commits from branch
- Resolved by Mayra Cabrera
@eurie could you please review this one? Thanks
I think we can skip product intelligence review. There is only one stub added to the tests
- Last reply by Vitali Tatarintev
assigned to @eurie
requested review from @eurie
removed product intelligencereview pending label
removed product intelligence label
assigned to @mayra-cabrera and unassigned @eurie
requested review from @mayra-cabrera and removed review request for @eurie
unassigned @mayra-cabrera
removed review request for @mayra-cabrera
added 687 commits
-
011417d6...3c90b746 - 684 commits from branch
master - 528fab7a - Ignore default_enabled arg in Feature.enabled?
- b9986fa2 - Add missing feature flag definition files
- 0b3162b9 - Adjust feature flag related tests
Toggle commit list-
011417d6...3c90b746 - 684 commits from branch
assigned to @mayra-cabrera and unassigned @ck3g
added product intelligence product intelligencereview pending labels
product intelligence
spec/requests/api/usage_data_spec.rbLGTM as this is a simple specs fix.added product intelligenceapproved label and removed product intelligencereview pending label
enabled an automatic merge when the pipeline for 0e149428 succeeds
mentioned in commit 0767b9e2
added workflowstaging label
mentioned in merge request !52486 (closed)
mentioned in issue #326201 (closed)
mentioned in issue gitlab-com/gl-infra/production#4091 (closed)
65 65 "The thing '#{thing.class.name}' for feature flag '#{key}' needs to include `FeatureGate` or implement `flipper_id`" 66 66 end 67 67 68 Feature::Definition.valid_usage!(key, type: type, default_enabled: default_enabled) 68 Feature::Definition.valid_usage!(key, type: type, default_enabled: :yaml) 69 69 end 70 70 71 # If `default_enabled: :yaml` we fetch the value from the YAML definition instead. 72 default_enabled = Feature::Definition.default_enabled?(key) if default_enabled == :yaml 71 # TODO: Remove rubocop disable comment once `default_enabled` argument is removed https://gitlab.com/gitlab-org/gitlab/-/issues/271275 72 default_enabled = Feature::Definition.default_enabled?(key) # rubocop:disable Lint/ShadowedArgument I believe that change of this line causes the gitlab-com/gl-infra/production#4091 (comment 540067804) this. Some feature flags, primarly
gitaly_*might not be reflect in all our development feature flags.This, this makes it fail.
For "cpu_duration" related metrics we are adding a bunch missing feature flag configurations in !57699 (closed).
Is this something we could do for gitaly as well?
I think we should move gitaly into separate bucket. I know that we had this discussion before: gitaly#3057 (closed).
8 8 9 9 before do 10 10 skip_feature_flags_yaml_validation 11 skip_default_enabled_yaml_check We should not skip that. Otherwise it would fail and raise exception about the gitlab-com/gl-infra/production#4091 (comment 540067804).
Ignores
default_enabledargument inFeature.enabled?. Takesdefault_enabledvalue from the feature flag definition file instead. That should ensure that everything works as expected withoutdefault_enabledargument.We cannot do it, as: In
shared.rbwe have some entries marked asoptional: true. It means that they will fail, as not havingdefault_enabled:.The likely proper way to solve that is:
def enabled?(key, thing = nil, type: :development, default_enabled: :yaml)And let all other that do not have
YAML(as optional) to definedefault_enabled: false/trueif needed.Edited by Kamil Trzciński
mentioned in commit 4aabd520
mentioned in merge request !57707 (merged)
-
The
package-and-qajob from pipeline https://gitlab.com/gitlab-org/gitlab/-/pipelines/277348415 triggered https://gitlab.com/gitlab-org/build/omnibus-gitlab-mirror/-/pipelines/278739495 downstream.
added workflowproduction label and removed workflowstaging label
added releasedcandidate label
removed typefeature label
added pipeline:run-all-rspec pipeline:run-as-if-foss labels
mentioned in merge request !85522 (merged)
