-
Notifications
You must be signed in to change notification settings - Fork 16
feat: manifest command group for adding and listing flags in manifest #168
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
base: main
Are you sure you want to change the base?
feat: manifest command group for adding and listing flags in manifest #168
Conversation
Summary of ChangesHello @georgesdugue, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the OpenFeature CLI by introducing a dedicated command group for managing feature flag manifests. It provides users with the ability to easily add new flags to a manifest, complete with type validation and duplicate checks, and to view existing flags in a clear, tabular format. This streamlines the process of defining and maintaining feature flags directly from the command line, improving developer experience and consistency. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces new manifest
commands for managing feature flags, including add
and list
. The implementation is solid and includes comprehensive tests. My review focuses on improving error handling robustness, code maintainability by removing magic numbers, and adhering to standard Go formatting conventions.
3bb2ab7
to
9d8a674
Compare
9d8a674
to
531200d
Compare
…nality Signed-off-by: Georges Dugué <[email protected]>
Signed-off-by: Georges Dugué <[email protected]>
531200d
to
620af57
Compare
"flags": {} | ||
}`, | ||
validateResult: func(t *testing.T, fs afero.Fs) { | ||
content, err := afero.ReadFile(fs, "flags.json") |
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'm loving all of these tests! Very robust testing!
This doesn't have to change, but just some food for thought:
a lot of these values like flags.json, existingManifest, flags: {}, etc. are all repeated in these tests and constant. They may be able to be pulled up into the t.Run below that executes validateResult
, and that might reduce the lines of code a bit, but it would trade off the ability to customize those values for different test cases if we ever wanted to.
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.
approved! we'll just need to address the pr lint and pr test failures @georgesdugue |
Thank you! Will address the failing checks soon. |
…' commands and lint fix Signed-off-by: Georges Dugué <[email protected]>
c73190d
to
f4dcd5f
Compare
It looks like @kriscoleman's PR #167 has the fix for the Generator Integration Tests failure. I can wait for it to merge and rebase. |
Hey @georgesdugue, I'll try and take a look at this tonight. Thanks! |
This PR
openfeature manifest
command group for managing flag manifest filesmanifest add
subcommand to add new flags to manifests with type validation and duplicate detectionmanifest list
subcommand to display all flags in a formatted table viewRelated Issues
Fixes #153
Notes
manifest add
command validates default values against the specified flag type before adding to the manifestFollow-up Tasks
None
How to test
Testing
manifest add
: