-
Notifications
You must be signed in to change notification settings - Fork 198
feat(feature): Implement Feature API #4039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
✅ Deploy Preview for zarf-docs canceled.
|
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits, otherwise looks like a solid strategy
src/internal/feature/feature.go
Outdated
|
||
// Set takes a slice of one or many flags, inserting the features onto user-configured features. If a feature name is | ||
// provided that is already a part of the set, then Set will return an error. | ||
// TODO: Should we allow users to call this multiple times even if we don't allow them to overwrite features? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dis-allowing is fine. If there's a world where we need to allow multiple sets in the future then we could change it, but this is probably the most intuitive option since there's always an error on the second attempt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i'm happy with the current behavior as well.
The main edge case I could see this creating is when a user is calling the SDK in one "context" with one set of feature flags, and then needs a different set of flags for a different context later but within the same running process. The workaround is just to enable everything up front, but issues could still arise. Simple enough to ease the restriction later though.
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Co-authored-by: Austin Abro <37223396+AustinAbro321@users.noreply.github.com> Signed-off-by: Kit Patella <kit@defenseunicorns.com>
3912dce
to
652e6cf
Compare
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor items for context or my education. Appreciative of the tests - they were easy to step through an observe intent.
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Kit Patella <kit@defenseunicorns.com> Co-authored-by: Austin Abro <37223396+AustinAbro321@users.noreply.github.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com> Co-authored-by: Austin Abro <37223396+AustinAbro321@users.noreply.github.com> Signed-off-by: Cade Thomas <cadethomas23@gmail.com>
Description
This PR implements the
internal/feature
API as described in ZEP-0035 Feature Flags. It creates a new packagefeature
which allows maintainers to set defaultFeature
s, and provides facilities for users to enable and disable features for use in the SDK. Before release we should ship at least one default feature flag, even a test/documentation noop feature struct, to enforce that setDefault can only be called once.In a follow-up PR we'll implement:
feature.SetUser
) will be applied in the cmd layer to allow users to configure their own features via CLI flags, Env vars, and on-disk config.Related Issue
Implements part of ZEP-0035
Checklist before merging