✨ Add feature gate for preflight permissions #1666
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1666 +/- ##
==========================================
+ Coverage 67.42% 67.43% +0.01%
==========================================
Files 55 57 +2
Lines 4632 4622 -10
==========================================
- Hits 3123 3117 -6
+ Misses 1284 1278 -6
- Partials 225 227 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/test upgrade-e2e |
|
@bentito: No presubmit jobs available for operator-framework/operator-controller@main In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
removed
the
do-not-merge/hold
What is DX? Dev Experience? I don't see how that's relevant. For this to be a problem for a user, they have to 1) find out how to enable a feature gated item, 2) have some expectation of what enabling that feature would do. That seems unlikely without documenting the flag, which will come later. The reason this PR is helpful to merge is that it allows other contributing devs to use the flag. There are lots of other ways that other devs could learn about and use this flag, but merging this PR is a straightforward manner, one that doesn't assume how the devs will communicate and share info. As soon as a dev uses the flag there will be at least one check hidden behind the flag. |
Signed-off-by: Brett Tofel <[email protected]>
|
Hi @bentito As discussed, we can add the option |
internal/features/features.go
Outdated
| ) | ||
|
|
||
| var operatorControllerFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ | ||
| // Add new feature gate definitions | ||
| // Ex: SomeFeature: {...} | ||
| PreflightPermissions: {Default: false, PreRelease: featuregate.Alpha}, |
There was a problem hiding this comment.
It is missing LockToDefault: true
There was a problem hiding this comment.
making it PreAlpha to settle all (most?) concerns
Signed-off-by: Brett Tofel <[email protected]>
| PreflightPermissions: { | ||
| Default: false, | ||
| PreRelease: featuregate.PreAlpha, // keep this PreAlpha until done with feature development | ||
| LockToDefault: false, |
There was a problem hiding this comment.
Should not be true?
There was a problem hiding this comment.
If we set LockToDefault to true, @ankitathomas reports that you cannot access the feature-gated code for testing by then setting the emulated version. So LockToDefault true would render it untestable.
There was a problem hiding this comment.
If we do not set it true:
- users will be able to opt-in / use a flag that do nothing
- then, we came back to the same concerns: ✨ Add feature gate for preflight permissions #1666 (comment)
Description
Allows this work to be done behind a feature gate, allowing for testing and for the code to settle in before it starts affecting operations.
Name of the feature gate is,
PreflightPermissions, see code and tests for usage.Should be a no-op even if enabled, as there is currently no code behind it.
Reviewer Checklist