feat: TOOLS-3092 Enhanced Diff Versions & List Versions Commands Functionality #84
Conversation
TOOLS-3092 Add list command to display available Aerospike server versions - Introduced a new `list` command with a subcommand `versions` to list all available Aerospike server versions. - Implemented verbose output option for detailed version listing. - Integrated schema loading to retrieve available versions dynamically.
- Introduced comprehensive unit tests for the `list` command and its `versions` subcommand. - Validated command behavior with various flag combinations and ensured proper error handling. - Implemented output checks for both simple and verbose formats, confirming expected version listings and formatting. - Added tests for help command output to verify help information is displayed correctly.
…r configurations - Introduced a new `versions` subcommand under the `diff` command to compare configuration files between two specified Aerospike server versions. - Enhanced error handling for argument validation specific to version comparisons. - Updated command documentation and examples for clarity on usage. - Added support for filtering and compact output options in the versions diff command. - Integrated schema loading to facilitate comparison of configuration changes between versions.
a-spiker
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the asconfig diff versions command with improved usability, dynamic schema handling, and comprehensive end-to-end testing. It changes the default output from compact to detailed/verbose mode, introduces a new --compact flag for minimal output, and adds a list versions command to show available Aerospike server versions.
Key Changes:
- Inverted default behavior: verbose/detailed output is now default, with
--compactflag for minimal output - Dynamic schema section extraction eliminates hardcoded section names, making the tool future-proof
- New
asconfig list versionscommand for displaying available Aerospike server versions - Comprehensive end-to-end test suite validates real schema differences across versions
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/diff.go | Added newDiffVersionsCmd() function with detailed help text and runVersionsDiff() for schema comparison logic |
| cmd/diff_utils.go | New file containing schema comparison engine, dynamic section extraction, and flexible output formatting |
| cmd/diff_versions_test.go | Comprehensive end-to-end tests validating version comparisons, output formats, and CLI integration |
| cmd/list.go | New command for listing available Aerospike server versions with verbose/simple output modes |
| cmd/list_test.go | Tests for list command functionality and output formatting |
| cmd/root.go | Added list command to root and improved flag error handling for help requests |
| cmd/utils.go | Added error constants for version diff validation |
| schema/schemamap.go | Added JSON file filtering to only load .json schema files |
| go.mod | Added jsondiff dependency and updated rogpeppe/go-internal |
| go.sum | Updated dependency checksums for new and updated packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated the `TestVersionsDiffEndToEnd`, `TestVersionsDiffCLIIntegration`, and `TestVersionsDiffSpecificChanges` tests to dynamically retrieve available Aerospike server versions from the schema. - Modified test cases to use the first, last, and consecutive versions for comparisons, ensuring tests remain valid as versions change. - Added additional hardcoded tests for stable versions to ensure consistent behavior across major releases. - Improved output consistency checks in the `TestListVersionsOutput` and `TestListVersionsCount` tests by dynamically determining expected version counts and ensuring stable versions are included in the output.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
==========================================
+ Coverage 76.15% 81.74% +5.59%
==========================================
Files 13 15 +2
Lines 1086 2038 +952
==========================================
+ Hits 827 1666 +839
- Misses 190 277 +87
- Partials 69 95 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Introduced a new function `printSectionBox` to create dynamically-sized boxes around section names in the output, ensuring a minimum width for better readability. - Updated the `printSectionChanges` function to utilize the new box formatting, improving the visual structure of section headers. - Corrected error variable naming for consistency in argument validation within the `runVersionsDiff` function.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
This is looking good! I like the output style. I left a few nitpicks and things to double check. Let me know when you have had a look. I'm mostly concerned with #84 (comment)
cmd/diff.go
Outdated
| # Diff two local yaml configuration files | ||
| asconfig diff files aerospike1.yaml aerospike2.yaml | ||
| # Diff a local .conf file against a running server | ||
| asconfig diff server -h 127.0.0.1:3000 aerospike.conf`, |
There was a problem hiding this comment.
Can you add an example for diffing versions?
cmd/diff.go
Outdated
| return errors.Join(errInvalidSchemaVersion, fmt.Errorf("schema for version %s not found", version2)) | ||
| } | ||
|
|
||
| var schemaLower, schemaUpper map[string]interface{} |
There was a problem hiding this comment.
nit: it looks like the style in this file for empty interface is "any"
| summary.TotalRemovals++ | ||
| case Modification: | ||
| sectionChanges.Modifications = append(sectionChanges.Modifications, change) | ||
| summary.TotalModified++ |
There was a problem hiding this comment.
Should have a default case with some error handling or panic.
cmd/diff_utils.go
Outdated
| if validSections[part] { | ||
| return part | ||
| } |
There was a problem hiding this comment.
Using the _, ok := validSections[part]; ok pattern would be more idomatic here, you could store an empty struct as the value in this map that way.
cmd/diff_utils.go
Outdated
| // Convert to JSON and back to get a clean, consistent representation | ||
| jsonBytes, err := json.Marshal(propMap) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Why is this extra conversion needed? Does it do some formatting or something?
There was a problem hiding this comment.
I had done this to ensure we get rid of invalid values for type conversion. I was being over cautious but since these are getting loaded from a valid json, this was a over engineering. I have re worked on this. I have stilled added ciruclar references in handling in case for nested path handling, but just to ensure we dont't crash, and are robust.
cmd/diff_utils.go
Outdated
| // Skip complex nested objects in non-verbose mode (except for simple properties) | ||
| if !options.Verbose && isComplexProperty(val) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Will this hide certain diffs? It feels like all diffs should be pointed out even in --compact mode. Maybe you could just print a simple message indicating that the field changed but not what changed.
There was a problem hiding this comment.
I was skipping complex fields, and was not handling nested structures, now I have added handling of all the nested properties accordingly.
| fmt.Fprintf(os.Stdout, "%s\n", bottomBorder) | ||
| } | ||
|
|
||
| // formatPath formats a JSON path into a more human-readable form. |
There was a problem hiding this comment.
This is consistent with asconfig's current style but in hindsight I wish we output json pointer/path like syntax. It is a commonly understood format and the server is moving towards using it for config related errors etc.
There was a problem hiding this comment.
Based on discussions with the support team, customers prefer a diff that’s easy to interpret in layman terms. While displaying the diff directly on JSON is highly effective and helps introduce the concept of schemas for configuration validation, it also inherently exposes portions of the underlying schema.
I'm looking forward on your yaml work, and later if schemas get deprecated, then we might need to rethink this whole implementation.
- Updated the `compareSchemas` function to utilize `any` type for improved type handling. - Introduced new utility functions for better array and property formatting, ensuring complex objects are displayed correctly in output. - Added validation for filter sections in the `runVersionsDiff` function to enhance error handling. - Expanded test coverage for formatting functions and array changes to ensure consistent output without raw map displays. - Improved overall readability and structure of the output for schema changes.
- Fixed test cases with valid filter path - Moved formatting strings to constants
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
- Updated `diff_utils.go` to improve variable naming and output formatting for configuration changes, including renaming version fields for clarity. - Added initialization of global variables in `diff_test.go` to ensure proper test setup. - Introduced a utility function to capture stdout during tests, enhancing output validation in `diff_versions_test.go`. - Updated test cases to reflect changes in output formatting, ensuring consistency in configuration change summaries.
- Updated `diff_test.go` to improve validation of expected additions, removals, and modifications in configuration changes. - Introduced new test cases for array removals and modifications, ensuring correct output formatting in both verbose and compact modes. - Enhanced the `printArrayChange` functions in `diff_utils.go` to handle modifications distinctly, displaying old and new values clearly. - Improved regex pattern matching in `list_test.go` for counting numbered lines in verbose output, ensuring accuracy in test assertions.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated `formatNumber` in `diff_utils.go` to handle integer types directly, eliminating float64 precision loss. - Adjusted expected output in `TestFormatNumber` in `diff_test.go` to reflect the new formatting logic for max int64 value.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if len(filterSections) == 0 { | ||
| return nil, errors.New("filter-path provided but no valid sections found") |
There was a problem hiding this comment.
The error message for parseFilterPath returns "filter-path provided but no valid sections found" when the input string only contains whitespace or empty strings after splitting. However, this could happen if a user provides something like --filter-path "," or --filter-path " ".
Consider a more descriptive error message that clarifies what went wrong: "filter-path provided but contains no valid section names (found only empty or whitespace values)"
| return nil, errors.New("filter-path provided but no valid sections found") | |
| return nil, errors.New("filter-path provided but contains no valid section names (found only empty or whitespace values)") |
| case float64: | ||
| // Handle special cases for zero | ||
| if v == 0 { | ||
| return "0" | ||
| } | ||
|
|
||
| // For very small decimal numbers, use limited precision | ||
| if v < 1 && v > -1 && v != 0 { | ||
| return fmt.Sprintf("%.6g", v) | ||
| } | ||
|
|
||
| // For integers (whole numbers), show them as integers without scientific notation | ||
| if v == float64(int64(v)) { | ||
| return fmt.Sprintf("%.0f", v) | ||
| } | ||
|
|
||
| // For decimal numbers, show with limited precision to avoid scientific notation | ||
| return fmt.Sprintf("%.6g", v) |
There was a problem hiding this comment.
[nitpick] The test TestFormatNumber includes test cases for infinity and NaN (lines 1904-1917), but the formatNumber function doesn't have special handling for these values. While Go's fmt.Sprintf will format them as "+Inf", "-Inf", and "NaN" by default, it would be more explicit and maintainable to have dedicated handling for these edge cases in the function itself.
Consider adding explicit checks:
case float64:
if math.IsNaN(v) {
return "NaN"
}
if math.IsInf(v, 1) {
return "+Inf"
}
if math.IsInf(v, -1) {
return "-Inf"
}
// ... rest of the logicThis makes the behavior explicit and self-documenting.
📋 Summary
This PR significantly enhances the
asconfigtool with two major additions:diff versionsfunctionality with improved usability, dynamic schema handling, and comprehensive linting fixeslist versionscommand to display available Aerospike server versions🎯 Key Changes
1. New
list versionsCommandasconfig list versionscommand to display all available Aerospike server versions--verbose/-v): Numbered list with headers and total countdiff versions2. Enhanced
diff versionsCommand--compact/-cflag for minimal output when needed (replaces old--verboseflag)--filter-path/-fflag to focus on specific configuration sections (e.g.,service,namespaces)3. Dynamic Schema Handling (No More Hardcoding!)
4. Improved Output Formatting
🎨 User Experience Improvements
✅ Validation
This PR transforms the
diff versionsfunctionality from a basic comparison tool into a comprehensive, user-friendly schema analysis system while adding essential version discovery capabilities through the newlist versionscommand.