-
Couldn't load subscription status.
- Fork 17
feat: push command #170
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: push command #170
Conversation
2e261ad to
00517ec
Compare
|
@beeme1mr will need to test this with your flagd project |
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.
Pull Request Overview
This PR introduces a new push command to the OpenFeature CLI that enables users to upload local flag configurations to remote flag management services via HTTP/HTTPS endpoints. The implementation provides the inverse functionality of the existing pull command with similar UX patterns.
- New push command with HTTP/HTTPS support and authentication
- Dry-run functionality for previewing changes before pushing
- Comprehensive validation and error handling with detailed messages
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/manifest/manage.go | Adds SaveToRemote function for HTTP/HTTPS uploads with authentication |
| internal/config/flags.go | Adds push-specific command flags and configuration getters |
| internal/cmd/root.go | Registers the new push command with the CLI |
| internal/cmd/push.go | Core push command implementation with validation and URL scheme handling |
| internal/cmd/push_test.go | Comprehensive test suite covering HTTP methods, authentication, and error scenarios |
| docs/commands/openfeature_push.md | Auto-generated documentation for the push command |
| docs/commands/openfeature.md | Updated main documentation to include push command reference |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
a122e46 to
d7082b5
Compare
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.
Pull Request Overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
d7082b5 to
edf7d67
Compare
|
/gemini review |
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 a push command, which is a great addition for managing flag configurations. The implementation is well-structured and follows the existing patterns in the CLI. The test coverage for the new command is comprehensive and covers various success and failure scenarios.
I've identified a few areas for improvement:
- There's an opportunity to make the configuration handling more efficient by removing redundant file reads.
- Adding validation for the HTTP method in the
pushcommand would improve robustness. - There's some code duplication that could be refactored to improve maintainability.
Overall, this is a solid contribution. My detailed comments are below.
| func getConfigValueWithFallback(value string, configKey string) string { | ||
| if value != "" { | ||
| return value | ||
| } | ||
|
|
||
| viper.SetConfigName(".openfeature") | ||
| viper.AddConfigPath(".") | ||
| if err := viper.ReadInConfig(); err != nil { | ||
| return "" | ||
| } | ||
| if !viper.IsSet(configKey) { | ||
| return "" | ||
| } | ||
| return viper.GetString(configKey) | ||
| } |
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.
The getConfigValueWithFallback function re-configures and re-reads the viper configuration on every call. This is inefficient as it causes redundant file I/O. The initializeConfig function, called in PreRunE, is already responsible for setting up viper and reading the configuration file once.
This helper function should simply retrieve the value from the already-configured viper instance. viper.GetString(key) will return an empty string if the key is not found, which preserves the intended fallback logic.
func getConfigValueWithFallback(value string, configKey string) string {
if value != "" {
return value
}
return viper.GetString(configKey)
}
internal/cmd/push.go
Outdated
| httpMethod := config.GetPushMethod(cmd) | ||
| dryRun := config.GetDryRun(cmd) |
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.
The httpMethod is retrieved from the config but not validated. The help text says only POST and PUT are supported, but the code will accept any string. This could lead to unexpected behavior if a user provides an unsupported method. It's better to validate the method and return a clear error message.
I suggest converting the method to uppercase and checking if it's one of the supported methods (POST or PUT). You will need to add "strings" to your imports for this change.
httpMethod := strings.ToUpper(config.GetPushMethod(cmd))
if httpMethod != "POST" && httpMethod != "PUT" {
return fmt.Errorf("invalid HTTP method: %s. Only POST and PUT are supported", config.GetPushMethod(cmd))
}
dryRun := config.GetDryRun(cmd)
internal/manifest/manage.go
Outdated
| type SourceFlag struct { | ||
| Key string `json:"key"` | ||
| Type string `json:"type"` | ||
| Description string `json:"description"` | ||
| DefaultValue any `json:"defaultValue"` | ||
| } |
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.
This SourceFlag struct is a duplicate of the one defined locally within LoadFromSourceFlags in internal/flagset/flagset.go. Duplicating struct definitions can lead to maintenance issues if they diverge.
To improve maintainability, consider defining SourceFlag as a public type in the flagset package and reusing it in both LoadFromSourceFlags and SaveToRemote.
|
@beeme1mr suggested:
|
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.
@beeme1mr would love your 👀 on this
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 this should be a sync API that contains both a pull and push API.
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 was originally thinking the push API would be for a single flag. I'll have to think about the implications of a bulk API like you've proposed. One challenge is dealing with partial successes. For example, one flag is successfully updated but another can't be because a property (say flag type) is immutable. I do think properties like flag type will be immutable is most flag management systems.
Let me mess around with this in my test app to try and identify any challenges we may run into.
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.
Pull Request Overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
internal/cmd/push_test.go
Outdated
| "key": "usernameMaxLength", | ||
| "type": "integer", | ||
| "description": "Maximum allowed length for usernames.", | ||
| "defaultValue": float64(50), // JSON unmarshals numbers as float64 |
Copilot
AI
Oct 22, 2025
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.
[nitpick] The comment indicates JSON unmarshals integers as float64, but the expected value uses an explicit float64 conversion. This is correct for JSON unmarshaling behavior, but the test should verify that the API correctly handles integer types. Consider documenting whether the remote API is expected to accept float64 for integer flag types or if type preservation is required.
| "defaultValue": float64(50), // JSON unmarshals numbers as float64 | |
| "defaultValue": 50, // Use integer to verify type preservation for integer flag |
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 this should be a sync API that contains both a pull and push API.
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/SuccessResponse' | ||
| '201': |
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 201 doesn't make sense if we're returning a response body.
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ErrorResponse' |
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.
NIT: The error example is invalid auth, which doesn't match a 400
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ErrorResponse' | ||
| '401': |
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.
Do you think we should also include a 403?
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.
yea, I think that's a good idea
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 was originally thinking the push API would be for a single flag. I'll have to think about the implications of a bulk API like you've proposed. One challenge is dealing with partial successes. For example, one flag is successfully updated but another can't be because a property (say flag type) is immutable. I do think properties like flag type will be immutable is most flag management systems.
Let me mess around with this in my test app to try and identify any challenges we may run into.
|
Adds a new command to the CLI that allows users to push local flag configurations to a remote flag management service over HTTP/HTTPS. It also prevents local file scheme usage to avoid overwriting local files and validates if flags have default values set. Signed-off-by: Kris Coleman <[email protected]>
a123db5 to
dde9855
Compare
Summary
This PR introduces a new
pushcommand to the OpenFeature CLI, providing the inverse functionality of the existingpullcommand. The push command allows users to upload local flag configurations to remote flag management services.Closes #166
Motivation
As discussed in #166, there has been considerable interest in having a push operation to complement the existing pull functionality. This implementation provides an opinionated first version focused on a straightforward use case: pushing to HTTP/HTTPS endpoints. This initial implementation will help inform the design of a future extensible plugin system.
Changes
Core Implementation
pushcommand (internal/cmd/push.go): Mirrors the structure and UX of thepullcommandinternal/manifest/manage.go): Handles HTTP/HTTPS uploads with authenticationinternal/config/flags.go): Added push-specific flags and gettersinternal/cmd/root.go): Integrated push command into the CLIFeatures
Testing
internal/cmd/push_test.go) covering:Documentation
Design Decisions
file://support: Local file operations are better handled by standard shell commands (cp, mv)Usage Examples
Testing
All tests pass:
Future Considerations
This implementation lays the groundwork for future extensibility:
Checklist
pullcommand UX