-
Notifications
You must be signed in to change notification settings - Fork 1
feat: TOOLS-3092 Introduce list-versions and diff versions commands for Aerospike configuration management #75
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?
Conversation
…ing Aerospike server - Introduced a new `--server` flag for the `diff` command to allow comparison of a local configuration file against the live configuration from a running Aerospike server. - Updated error handling for argument counts specific to server diff. - Refactored the diff command to separate file-to-file and server-to-file comparison logic.
…ent validation - Introduced new test cases for the `--server` flag in the diff command, covering various scenarios including missing arguments, invalid formats, and non-existent files. - Added validation tests for server diff arguments to ensure proper error handling. - Enhanced existing tests for file diff functionality to include edge cases and argument validation.
- Removed outdated versions of aerospike-management-lib from go.sum to streamline dependencies and ensure compatibility with the latest library versions.
Replaces reflect.DeepEqual with a custom valuesEqual function for more robust type-aware comparison, handling cases like int vs string, slices, and booleans. Ensures server and local configs are parsed through the same path to normalize data types before diffing, improving accuracy of configuration diffs.
Refactored the valuesEqual function to compare values by string conversion first, improving type flexibility and preventing panics on slice comparison. Added extensive unit tests for valuesEqual, isSlice, slicesEqual, diffFlatMaps, and argument validation in diff_test.go to ensure correctness and robustness of diff logic.
Refactored slicesEqual to compare slices in an order-agnostic way using frequency counting for O(n) complexity. Added convertToStringSlice to handle different slice types and ensure consistent comparison. Updated tests to validate new behavior, including mixed types and order variations.
Introduces a warning message indicating the server diff feature is in beta Since the generate command is in beta and diff with server uses generate functionality
Refactors the diff command to use a subcommand structure, moving server diff functionality to a new 'diff server' subcommand. Simplifies flag handling, updates help text and examples, and streamlines the diff logic by removing custom value comparison in favor of reflect.DeepEqual.
This commit removes extensive low-level unit tests for helper functions such as valuesEqual, isSlice, and slicesEqual, as well as redundant server diff argument validation and flag validation tests. The remaining tests focus on higher-level command behavior and integration, streamlining the test suite and reducing maintenance overhead.
Expanded the short and long descriptions of the diff command to clarify support for comparing a config file against a running server. Marked the server comparison feature as BETA in the server subcommand descriptions.
Improved the short and long descriptions for the diff and server subcommands to clarify that comparisons are made against a running server's configuration. Updated examples and added notes about supported file formats for better user guidance.
Moved the --format flag from persistent to local flags in the diff command and hid it in the server diff subcommand, as the format will be auto-detected. Added a debug log to show the detected local file format.
Introduces 'files' and 'server' subcommands for the diff command, improving clarity and extensibility. The legacy behavior is preserved for backward compatibility, with a warning message. Adds tests for both legacy and new subcommand usage, including argument validation.
Changed the diff test assertion to check if the output contains the expected result instead of requiring an exact match. This allows for additional warnings or messages in the output without failing the test.
Improved the help text and examples for the diff, diff files, and diff server commands to clarify usage, supported formats, and default behaviors. Added more detailed flag descriptions and updated example host/port formats for consistency.
update server diff usage example Co-authored-by: dwelch-spike <[email protected]>
Introduces a new CLI command `list-versions` to display available Aerospike server versions, with optional table formatting. Adds corresponding unit tests and updates schema loading logic to only include JSON files.
…ions command Updated the list-versions command to replace the "table" flag with "verbose" for output formatting. Adjusted corresponding test cases to reflect this change, ensuring consistency in command usage.
… configurations Introduces a new command `diff versions` to compare configuration changes between two specified Aerospike server versions. This feature includes validation for version order, schema loading, and detailed output options for added, removed, or modified configurations. Updates the command structure to enhance usability and maintainability.
dwelch-spike
left a comment
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 is a really useful addition to asconfig, nice work! I have some thoughts about the implementation that I left in comments, mainly...
I'm worried that some of the formatting, printing, and parsing is too tightly coupled to values in the schemas. Those values can and have changed, sometimes we add new ones etc. If possible, we should try to reduce the coupling to concrete fields in the schema and generate output dynamically. Let me know what you think, if this introduces too much complexity we can do something else.
It might be worth checking out this library https://github.com/josephburnett/jd it seems very popular and looks like it can generate diff output.
Nice work, let me know when you want me to take another look.
| func newDiffVersionsCmd() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "versions [flags] <version1> <version2>", | ||
| Short: "Diff Aerospike server versions.", |
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.
| Short: "Diff Aerospike server versions.", | |
| Short: "Show configuration file difference between versions of the Aerospike server.", |
suggestion: thinking about how to clearly describe this
| } | ||
|
|
||
| // isVersionLessEqual compares two version strings (X.X.X) and returns true if v1 <= v2 | ||
| func isVersionLessEqual(v1, v2 string) bool { |
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.
Alot of our tools have their own implementation of this. Could we use the one in the management liv here? https://github.com/aerospike/aerospike-management-lib/blob/504da4668525110dd75aa2bd5aa33d52a12e8233/utils.go#L72
If not, we should add this to tools common go so we can unify how our tools compare versions.
|
|
||
| func newListVersionsCmd() *cobra.Command { | ||
| res := &cobra.Command{ | ||
| Use: "list-versions", |
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.
what do you think of adding a list command and "versions" as a subcommand, so that the useage becomes list versions that way we can easily add more listings later.
| } | ||
|
|
||
| // compareSchemas compares two schema objects and returns a summary of changes | ||
| func compareSchemas(file1Obj, file2Obj map[string]interface{}, newVersion, oldVersion string) (ChangeSummary, error) { |
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.
| func compareSchemas(file1Obj, file2Obj map[string]interface{}, newVersion, oldVersion string) (ChangeSummary, error) { | |
| func compareSchemas(schema_left, schema_right map[string]interface{}, newVersion, oldVersion string) (ChangeSummary, error) { |
nit: I think these names could be clearer. maybe "schema_old, schema_new" ?
| for _, change := range changes { | ||
| section := extractSection(change.Path) | ||
|
|
||
| sectionChanges := summary.Sections[section] |
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.
won't this panic since summary.Sections[section] does not exist yet? I'd just initialize it to a new empty object
| // Define valid top-level sections | ||
| validSections := map[string]bool{ | ||
| "service": true, | ||
| "logging": true, | ||
| "network": true, | ||
| "namespaces": true, | ||
| "mod-lua": true, | ||
| "security": true, | ||
| "xdr": true, | ||
| } | ||
|
|
||
| // Look for valid top-level section in the path | ||
| for _, part := range parts { | ||
| if part == "" || part == "properties" || part == "items" || isNumeric(part) { | ||
| continue | ||
| } |
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.
It would be nice to avoid hardcoded tables like this so they don't need to be updated when the schemas/configs change. Maybe we could use the first element in the json path as the section and then exclude items we know we don't want? What do you think?
| sectionOrder := []string{ | ||
| "service", | ||
| "logging", | ||
| "network", | ||
| "namespaces", | ||
| "mod-lua", | ||
| "security", | ||
| "xdr", | ||
| "general", | ||
| } |
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.
It's nice to avoid hardcoding these where possible, since they may change. Can we sort alphabetically or something similar instead?
| } | ||
|
|
||
| // printValueProperties prints properties of a value | ||
| func printValueProperties(value interface{}, options DiffOptions) { |
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.
If we ever introduce new values to the schema, they will need to be added here. Is there some way we can generate this text dynamically? For example, by marshalling to json/yaml or a custom format?
| // In verbose mode, print additional properties | ||
| // Print type information | ||
| if typeVal, exists := propMap["type"]; exists { | ||
| fmt.Printf("%sType: %v\n", prefix, typeVal) | ||
| } | ||
|
|
||
| // Print min/max values | ||
| if minVal, exists := propMap["minimum"]; exists { | ||
| fmt.Printf("%sMinimum: %v\n", prefix, minVal) | ||
| } | ||
| if maxVal, exists := propMap["maximum"]; exists { | ||
| fmt.Printf("%sMaximum: %v\n", prefix, maxVal) | ||
| } | ||
|
|
||
| // Print enum values if they exist | ||
| if enumVals, exists := propMap["enum"].([]interface{}); exists { | ||
| fmt.Printf("%sAllowed values: %v\n", prefix, enumVals) | ||
| } |
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.
Are these value properties or object properties?
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/wI2L/jsondiff" |
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.
Maybe worth taking a look at https://github.com/josephburnett/jd it has more stars and seems to generate diffs as text output. It might be worth experimenting with, maybe it could simplify this code.
tanmayja
left a comment
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.
can we have some tests around these commands?
| } | ||
|
|
||
| cmd.Flags().BoolP("verbose", "v", false, "Show detailed information about property changes (type, default values, etc.)") | ||
| cmd.Flags().StringP("filter-path", "f", "", "Filter results to only show properties under the specified path (e.g., 'service', 'namespaces')") |
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.
Does it take nested path also? or just the main context?
|
@dwelch-spike I had few things in for this feature, let me know if they sounds good.
|
|
Due to linting overhaul, this PR will be discarded, changes now part of #84 |
Summary:
This PR significantly enhances the
asconfigCLI by introducing two new commands designed to improve Aerospike configuration analysis and management:list-versionsCommand:asconfig list-versionshas been added.--verbose(or-v) flag that, when enabled, displays the output in a detailed, numbered format, providing a clear overview of available versions.diff versionsCommand:asconfig diff versions <version1> <version2>has been implemented.These new commands provide essential tools for Aerospike administrators and developers to manage and understand configuration changes more effectively.