feat(cli): add manifest delete command #191
Conversation
Summary of ChangesHello @reckziegelwilliam, 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 introduces a new 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a manifest delete subcommand, which is a great addition for managing the flag lifecycle via the CLI. The implementation is clean, follows existing patterns in the codebase, and reuses helpers for file I/O, which is excellent. The test suite is particularly impressive for its comprehensiveness, covering a wide range of success and error scenarios. I have one suggestion to improve the test validation logic to make it even more robust and maintainable by using the application's own abstractions instead of parsing raw JSON.
| validateResult: func(t *testing.T, fs afero.Fs) { | ||
| content, err := afero.ReadFile(fs, "flags.json") | ||
| require.NoError(t, err) | ||
|
|
||
| var manifest map[string]interface{} | ||
| err = json.Unmarshal(content, &manifest) | ||
| require.NoError(t, err) | ||
|
|
||
| flags := manifest["flags"].(map[string]interface{}) | ||
| assert.Len(t, flags, 2, "Should have 2 flags remaining") | ||
| assert.Contains(t, flags, "flag-to-keep") | ||
| assert.Contains(t, flags, "another-flag") | ||
| assert.NotContains(t, flags, "flag-to-delete", "Deleted flag should not be present") | ||
| }, |
There was a problem hiding this comment.
The test validation logic is tightly coupled to the raw JSON structure by unmarshalling into a map[string]interface{}. It would be more robust and maintainable to use the manifest.LoadFlagSet function to parse the resulting manifest. This ensures the test validates the file using the same logic as the command itself, decoupling the test from the underlying file format and focusing on the behavior of the abstraction. This pattern can be applied to the other test cases in this file as well.
validateResult: func(t *testing.T, fs afero.Fs) {
fsAfter, err := manifest.LoadFlagSet("flags.json")
require.NoError(t, err)
assert.Len(t, fsAfter.Flags, 2, "Should have 2 flags remaining")
flagKeys := make(map[string]bool)
for _, flag := range fsAfter.Flags {
flagKeys[flag.Key] = true
}
assert.Contains(t, flagKeys, "flag-to-keep")
assert.Contains(t, flagKeys, "another-flag")
assert.NotContains(t, flagKeys, "flag-to-delete", "Deleted flag should not be present")
},There was a problem hiding this comment.
I'll wait for maintainer feedback before making changes to keep the review focused. Happy to refactor the tests if the maintainers agree this would be beneficial.
@beeme1mr - let me know your thoughts on this suggestion!
There was a problem hiding this comment.
It's a minor nitpick, searching the code base I see map[string]interface{} being used elsewhere, so we're not 100% consistent with it already.
I do agree it's better practice and might make the test less brittle, but I'm ok with this as-is imho.
Implement manifest delete subcommand to allow users to remove flags from their manifest file via the CLI. This complements the existing add subcommand and provides complete flag lifecycle management. Features: - Command validates flag exists before deletion - Uses atomic temp file pattern for safe I/O operations - Provides clear success/error feedback - Follows existing CLI patterns and conventions - Includes comprehensive unit tests (9 test cases) - Handles edge cases (empty manifest, non-existent flags, etc.) Fixes open-feature#154 Signed-off-by: Liam <[email protected]>
reckziegelwilliam
force-pushed
the
feat/manifest-delete
branch
from
f6b98a4 to
9f4ebf9
Compare
| } | ||
|
|
||
| // Remove the flag by creating a new slice without it | ||
| fs.Flags = append(fs.Flags[:flagIndex], fs.Flags[flagIndex+1:]...) |
There was a problem hiding this comment.
I dig this one-liner, elegant!
There was a problem hiding this comment.
slices.DeleteFunc will be a better option here.
There was a problem hiding this comment.
You can see how I used it here in the go-sdk:
https://github.com/open-feature/go-sdk/blob/794d397d0f0c47fd917f80e4e5114a71100246cf/openfeature/event_executor.go#L94
There was a problem hiding this comment.
any reason to use slices.DeleteFunc over slices.Delete here?
I didn't mind the append pattern but if we want to change it, .Delete seems fine here over .DeleteFunc
There was a problem hiding this comment.
I added a suggestion using slices.DeleteFunc. It allows us to get rid of the for loop entirely.
There was a problem hiding this comment.
I am a fan of the test cases here, you covered a lot of edge cases 🏅
|
Looking great to me so far! thanks @reckziegelwilliam I request that we update the docs:
|
| // Check if manifest has any flags | ||
| if len(fs.Flags) == 0 { | ||
| return fmt.Errorf("manifest contains no flags") | ||
| } | ||
|
|
||
| // Check if flag exists | ||
| flagIndex := -1 | ||
| for i, flag := range fs.Flags { | ||
| if flag.Key == flagName { | ||
| flagIndex = i | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if flagIndex == -1 { | ||
| return fmt.Errorf("flag '%s' not found in manifest", flagName) | ||
| } | ||
|
|
||
| // Remove the flag by creating a new slice without it | ||
| fs.Flags = append(fs.Flags[:flagIndex], fs.Flags[flagIndex+1:]...) |
There was a problem hiding this comment.
| // Check if manifest has any flags | |
| if len(fs.Flags) == 0 { | |
| return fmt.Errorf("manifest contains no flags") | |
| } | |
| // Check if flag exists | |
| flagIndex := -1 | |
| for i, flag := range fs.Flags { | |
| if flag.Key == flagName { | |
| flagIndex = i | |
| break | |
| } | |
| } | |
| if flagIndex == -1 { | |
| return fmt.Errorf("flag '%s' not found in manifest", flagName) | |
| } | |
| // Remove the flag by creating a new slice without it | |
| fs.Flags = append(fs.Flags[:flagIndex], fs.Flags[flagIndex+1:]...) | |
| // Remove the flag | |
| originalLen := len(fs.Flags) | |
| fs.Flags = slices.DeleteFunc(fs.Flags, func(flag flagset.Flag) bool { | |
| return flag.Key == flagName | |
| }) | |
| // Check if flag was found (length unchanged means nothing was deleted) | |
| if len(fs.Flags) == originalLen { | |
| return fmt.Errorf("flag '%s' not found in manifest", flagName) | |
| } |
The zero length check can be skipped as it's not really an edge case and is covered by the "flag not found in manifest" check.
|
Do we need to handle the case where the flag being deleted exists more than once in the manifest? Or is that impossible? |
It is handled in our add command: cli/internal/cmd/manifest_add.go Line 167 in b255228 However, it might be worth adding it to our cli/internal/manifest/validate.go Line 18 in b255228 https://github.com/search?q=repo%3Aopen-feature%2Fcli%20manifest.LoadFlagSet&type=code This way, if someone adds the flag manually by editing the json for instance, the next usage of the cli against the manifest would throw invalidation errors immediately. I would support that, but suggest in another PR for hygiene. |
I added #193 |
4 participants
Implement manifest delete subcommand to allow users to remove flags from their manifest file via the CLI. This complements the existing add subcommand and provides complete flag lifecycle management.
Features:
Fixes #154
This PR
openfeature manifest delete <flag-name>subcommandmanifest.Write()) for safe I/O operationsmanifest addcommandFiles Added:
internal/cmd/manifest_delete.go- implements the delete command (91 lines)internal/cmd/manifest_delete_test.go- comprehensive test suite (464 lines)Files Modified:
internal/cmd/manifest.go- registers the delete subcommand (1 line added)internal/config/flags.go- addsAddManifestDeleteFlags()helper for consistency (4 lines added)Related Issues
Fixes #154
Notes
manifest.Write()function which already implements the atomic temp file pattern (write to temp → close → rename), ensuring no data corruption occursmanifest addandmanifest listcommands--manifestflag for custom manifest pathsmanifest addfor consistencyFollow-up Tasks
None - all acceptance criteria from issue #154 have been fully implemented and tested.
How to test
1. Automated Tests
Run all tests
go test ./...
Run specific manifest delete tests
go test ./internal/cmd/... -v -run TestManifestDelete
Verify build
go build ./cmd/openfeature2. Manual Testing
Create a test manifest:
cat > test-flags.json << 'EOF'
{
"$schema": "https://raw.githubusercontent.com/open-feature/cli/refs/heads/main/schema/v0/flag-manifest.json",
"flags": {
"feature-a": {
"flagType": "boolean",
"defaultValue": true,
"description": "First feature flag"
},
"feature-b": {
"flagType": "string",
"defaultValue": "test",
"description": "Second feature flag"
},
"feature-c": {
"flagType": "integer",
"defaultValue": 42,
"description": "Third feature flag"
}
}
}
EOFTest basic deletion:
Delete a flag
./openfeature manifest delete feature-a --manifest test-flags.json
Verify the flag was removed
cat test-flags.json
Should show only feature-b and feature-c remain
Delete another flag
./openfeature manifest delete feature-b --manifest test-flags.json
Delete the last flag
./openfeature manifest delete feature-c --manifest test-flags.json
Results in empty flags object: {"flags": {}}Test error handling:
Try to delete non-existent flag
./openfeature manifest delete non-existent --manifest test-flags.json
Error: flag 'non-existent' not found in manifest
Try to delete from empty manifest
echo '{"$schema": "...", "flags": {}}' > empty.json
./openfeature manifest delete any-flag --manifest empty.json
Error: manifest contains no flags
Try to delete from non-existent file
./openfeature manifest delete any-flag --manifest missing.json
Error: manifest file does not existTest with debug logging:
./openfeature manifest delete feature-a --manifest test-flags.json --debug
Should show: DEBUG Deleted flag: name=feature-a, manifestPath=test-flags.jsonView help:
./openfeature manifest delete --help
Shows usage, examples, and flag options3. Test Coverage Verification
The test suite covers: