feat(tools/confix): upgrade confix to migrate client.toml files #18455
Conversation
WalkthroughWalkthroughThe changes across the Changes
TipsChat with CodeRabbit Bot (
|
github-prbot
requested review from
a team,
tac0turtle and
atheeshp
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- tools/confix/data/v0.47-client.toml
- tools/confix/data/v0.50-client.toml
- tools/confix/data/v0.51-client.toml
Files selected for processing (6)
- tools/confix/README.md (1 hunks)
- tools/confix/cmd/diff.go (1 hunks)
- tools/confix/cmd/migrate.go (3 hunks)
- tools/confix/cmd/migrate_test.go (1 hunks)
- tools/confix/file.go (2 hunks)
- tools/confix/migrations.go (3 hunks)
Additional comments: 9
tools/confix/README.md (1)
- 94-110: The documentation has been updated to reflect the new usage of the
confix migratecommand, which now requires specifying the type of configuration file (clientorapp). This is a significant change for users who will need to adjust their commands accordingly. The examples provided are clear and show how to use the command for bothapp.tomlandclient.tomlfiles.tools/confix/cmd/migrate_test.go (1)
- 19-39: The tests have been updated to reflect the new functionality of handling different types of configuration files (
appandclient). The tests now include additional arguments to specify the configuration type and check for specific error messages and output strings. This ensures that the migration command behaves as expected when dealing with different configuration types and scenarios.tools/confix/file.go (1)
- 42-52: The
getFileNamefunction is a clean and maintainable way to construct the filename based on the configuration type. The use ofstrings.ToLowerensures that theconfigTypeargument is case-insensitive, which is user-friendly. However, it's important to ensure that all possibleconfigTypevalues are handled, and that the error message is clear to the user about what values are acceptable.tools/confix/migrations.go (1)
- 19-20: The function signature in the
MigrationMaptype has been updated to include an additionalplanTypeparameter. This change will require all functions that are used as values in this map to have the same updated signature.tools/confix/cmd/diff.go (2)
18-21: The command usage string and description have been updated to reflect the new
config-typeargument. This is a user-facing change and should be clearly documented in theREADME.mdand any other user guides.22-47: The
RunEfunction has been updated to handle a newconfig-typeargument. The default value forconfigTypeis set to "app", and there is a check to ensure that if provided, it must be either "app" or "client". This is a good approach to maintain backward compatibility and enforce input validation.tools/confix/cmd/migrate.go (3)
23-27: The command usage string and description have been updated to reflect the new functionality. It is now clear that the command can handle different types of configuration files, which is a significant user-facing change. Ensure that all documentation, help commands, and user guides are updated to reflect this new usage pattern.
28-28: The
cobra.MinimumNArgs(2)ensures that at least two arguments are provided to the command, which aligns with the new requirement for an additionalconfig-typeargument. This is a good use of Cobra's built-in argument validation.62-62: The
confix.Upgradefunction is now being called with the newconfigTypeargument, which is necessary for the enhanced functionality. This change should be checked to ensure that theUpgradefunction and any other downstream functions that depend on it are properly handling this new argument.
tools/confix/file.go
Outdated
| func LoadLocalConfig(name string, configType string) (*tomledit.Document, error) { | ||
| fileName, err := getFileName(name, configType) | ||
| if err != nil { | ||
| panic(fmt.Errorf("failed to read file: %w. This file should have been included in confix", err)) | ||
| return nil, err | ||
| } | ||
|
|
||
| f, err := data.Open(fmt.Sprintf("data/%s", fileName)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read file: %w. This file should have been included in confix", err) | ||
| } | ||
| defer f.Close() | ||
|
|
There was a problem hiding this comment.
The refactoring of LoadLocalConfig to include a configType parameter and use the new getFileName function is a good approach to handle different types of configuration files. The error handling is also improved by returning errors instead of panicking, which is a best practice for error handling in Go. This allows the caller to decide how to handle the error, rather than the application crashing.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- tools/confix/data/v0.51-client.toml
Files selected for processing (2)
- tools/confix/cmd/migrate_test.go (1 hunks)
- tools/confix/migrations.go (2 hunks)
Additional comments: 1
tools/confix/cmd/migrate_test.go (1)
- 19-39: The test cases have been updated to include the new
config-typeargument. Ensure that all test cases are updated accordingly and that they cover all new logic paths introduced by the changes.
tools/confix/migrations.go
Outdated
| target, err := LoadLocalConfig(to,planType) | ||
| if err != nil { | ||
| panic(fmt.Errorf("failed to parse file: %w. This file should have been valid", err)) |
There was a problem hiding this comment.
The previous comment about error handling in PlanBuilder is still valid. The function should not panic but rather return an error. This is crucial for robust error handling and to avoid crashing the application.
- func PlanBuilder(from *tomledit.Document, to string, planType string) transform.Plan {
+ func PlanBuilder(from *tomledit.Document, to string, planType string) (transform.Plan, error) {
- panic(fmt.Errorf("failed to parse file: %w. This file should have been valid", err))
+ return transform.Plan{}, fmt.Errorf("failed to parse file: %w. This file should have been valid", err)
}And then update the call sites of PlanBuilder to handle the returned error.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| target, err := LoadLocalConfig(to,planType) | |
| if err != nil { | |
| panic(fmt.Errorf("failed to parse file: %w. This file should have been valid", err)) | |
| func PlanBuilder(from *tomledit.Document, to string, planType string) (transform.Plan, error) { | |
| target, err := LoadLocalConfig(to, planType) | |
| if err != nil { | |
| return transform.Plan{}, fmt.Errorf("failed to parse file: %w. This file should have been valid", err) | |
| } |
Default to app.toml, add constants for config types, remove gas-adjustment in v0.51 config
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- tools/confix/data/v0.51-client.toml
Files selected for processing (2)
- tools/confix/cmd/migrate.go (3 hunks)
- tools/confix/cmd/migrate_test.go (1 hunks)
Additional comments: 12
tools/confix/cmd/migrate_test.go (6)
9-40: The test cases have been updated to include the new
config-typeargument and a new versionv0.51. Ensure that the new argument is correctly used in all relevant function calls and that the test cases cover all possible scenarios, including failure cases for the new functionality.19-20: This test case checks for an "unknown version" error. Verify that the versioning system in the application correctly identifies and handles unknown versions.
23-24: This test case ensures that an error is thrown when the
app.tomlfile does not exist. Verify that the error handling for missing files is consistent across different configuration types.26-28: This test case checks for a failure when trying to migrate
client.tomlwithout the--skip-validateflag. Verify that the validation step is working as intended and that the error message is clear and informative.30-34: This test case checks for a successful migration with the
--skip-validateflag and the--verboseflag. Verify that the verbose output contains the expected information and that the migration process completes without errors.36-39: This test case is for a successful migration of
client.tomlusing the new versionv0.51. Verify that the migration process for the new version is working correctly and that the output contains the expected changes.tools/confix/cmd/migrate.go (6)
4-18: The introduction of a new constant
ClientConfigTypeis a good practice for maintainability and readability. It allows for easy reference and avoids hardcoding strings throughout the code.26-54: The
MigrateCommandfunction has been updated to accept a newconfig-typeargument. The default behavior is preserved by settingconfigTypetoAppConfigTypeif no third argument is provided. This is a good approach to maintain backward compatibility.39-42: The validation of
configTypeensures that only 'app' or 'client' are accepted. This is a good security practice to prevent unexpected behavior or errors.46-48: The error message provided when an unknown version is specified is informative and user-friendly, as it lists the supported versions. This is a good practice for UX.
51-53: The error handling when loading the configuration file is robust, wrapping the underlying error with additional context. This is good for maintainability and debugging.
58-68: The
confix.Upgradefunction call has been correctly updated to pass the newconfigTypeargument. The context is also being set up with a logger ifFlagVerboseis true, which is a good practice for debugging and monitoring.Overall, the changes in
tools/confix/cmd/migrate.goare well-implemented, with proper error handling, validation, and backward compatibility. The code is clean, readable, and follows good practices.
julienrbrt
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/confix/cmd/migrate.go (3 hunks)
Additional comments: 3
tools/confix/cmd/migrate.go (3)
4-19: The addition of the
stringspackage and the new constantsAppConfigTypeandClientConfigTypeare appropriate for the new functionality being introduced. These constants provide a clear and maintainable way to refer to the different configuration types.27-73: The
MigrateCommandfunction has been updated to handle an optional third argument for the configuration type. The default behavior is maintained by settingconfigTypetoAppConfigTypeif no third argument is provided. The error handling for an invalidconfigTypeis also correctly implemented. However, there is a redundant block of code that checks theconfigTypeand throws an error if it's not valid. This block is duplicated and should be removed to avoid unnecessary repetition.* 77-87: The `confix.Upgrade` function is correctly called with the new `configType` parameter. The context is also being set up to log to stderr if `FlagVerbose` is true. The error handling for the `confix.Upgrade` function is appropriate, wrapping the error to provide more context. </blockquote></details></blockquote></details> </details>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/confix/cmd/migrate.go (3 hunks)
Additional comments: 2
tools/confix/cmd/migrate.go (2)
4-19: The addition of a new constant
ClientConfigTypeis a good practice for maintainability and readability. It allows for easy reference and avoids hardcoding strings throughout the code.71-81: The
confix.Upgradefunction is now being called with additional parameters, which is consistent with the changes made to support different configuration types. The error handling and context setup for verbose logging are correctly implemented.
| switch { | ||
| case len(args) > 1: | ||
| filename = args[1] | ||
| configPath = args[1] | ||
| case clientCtx.HomeDir != "": | ||
| filename = fmt.Sprintf("%s/config/app.toml", clientCtx.HomeDir) | ||
| configPath = fmt.Sprintf("%s/config/%s.toml",clientCtx.HomeDir, configType) | ||
| default: | ||
| return errors.New("must provide a path to the app.toml file") | ||
| return errors.New("must provide a path to the config file") |
There was a problem hiding this comment.
The logic for determining the configPath is sound, but there is a potential issue with the switch statement. It does not have an expression to switch on, which is not valid syntax in Go. This should be corrected to an if statement or the switch should be given an appropriate expression.
- switch {
+ if len(args) > 1 {Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| switch { | |
| case len(args) > 1: | |
| filename = args[1] | |
| configPath = args[1] | |
| case clientCtx.HomeDir != "": | |
| filename = fmt.Sprintf("%s/config/app.toml", clientCtx.HomeDir) | |
| configPath = fmt.Sprintf("%s/config/%s.toml",clientCtx.HomeDir, configType) | |
| default: | |
| return errors.New("must provide a path to the app.toml file") | |
| return errors.New("must provide a path to the config file") | |
| if len(args) > 1 { | |
| configPath = args[1] | |
| case clientCtx.HomeDir != "": | |
| configPath = fmt.Sprintf("%s/config/%s.toml",clientCtx.HomeDir, configType) | |
| default: | |
| return errors.New("must provide a path to the config file") |
There was a problem hiding this comment.
That advice is not correct, Go allows for more flexible switch statements such as has been included in this commit.
There was a problem hiding this comment.
@odeke-em You are absolutely right, and I appreciate the correction. Go does indeed support "tagless" switch statements, which evaluate cases as boolean expressions. My apologies for the oversight. Thank you for pointing that out!
|
Thanks for the valuable feedback @odeke-em ! I made the changes as you requested. I hope it looks okay. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (5)
- tools/confix/cmd/diff.go (1 hunks)
- tools/confix/cmd/migrate.go (3 hunks)
- tools/confix/cmd/migrate_test.go (1 hunks)
- tools/confix/file.go (2 hunks)
- tools/confix/migrations.go (2 hunks)
Additional comments: 12
tools/confix/cmd/diff.go (1)
- 3-48: The
DiffCommandfunction has been updated to handle a newconfig-typeargument. This allows users to specify the type of configuration file they want to compare, which can be eitherapp.tomlorclient.toml. The default behavior is to assumeapp.tomlif noconfig-typeis specified. Error handling has been added to ensure that an unsupportedconfig-typeresults in an error message. The function also checks if the specified target version exists in theMigrationsmap and loads the appropriate local configuration file based on theconfig-type. However, there is a use ofpanicfor error handling which should be replaced with a proper error return.</blockquote></details> <details><summary>tools/confix/file.go (1)</summary><blockquote> * 43-53: The `getFileName` function is a good addition for constructing filenames based on the configuration type. It uses a switch statement to handle different types of configurations, which is a clean and maintainable approach. However, ensure that the `configType` strings ("app" and "client") are consistently used throughout the codebase to avoid any mismatches or typos. </blockquote></details> <details><summary>tools/confix/cmd/migrate_test.go (6)</summary><blockquote> * 3-13: The import renaming from `fmt` to `path/filepath` is appropriate given the context of file path manipulations in the test cases. * 19-20: The test case checks for an error when an unknown version is provided. This is a good test for validating the error handling of the `MigrateCommand`. * 23-24: This test case ensures that an error is thrown when the `app.toml` file does not exist. It's important to verify that the error message is clear and actionable for the user. * 27-28: The test case checks for an error when attempting to migrate a `client.toml` without the `--skip-validate` flag. This is a good test to ensure that the validation step is not bypassed unintentionally. * 31-33: This test case verifies that the migration can proceed with the `--skip-validate` flag and that the output contains expected changes. It's important to ensure that the `--verbose` flag is functioning as intended and providing detailed output. * 36-39: The test case checks the migration of a `client.toml` file to version `v0.51` with verbose output. It's crucial to ensure that the new functionality for handling different config types is tested thoroughly. </blockquote></details> <details><summary>tools/confix/migrations.go (2)</summary><blockquote> * 13-19: The constants defined here are clear and well-named, indicating their purpose and usage within the code. This is good practice for maintainability and readability. * 22-22: The `MigrationMap` type has been updated to include a `planType` parameter in the function signature. This change should be cross-checked with all usages of `MigrationMap` to ensure they are updated accordingly. </blockquote></details> <details><summary>tools/confix/cmd/migrate.go (2)</summary><blockquote> * 4-19: The addition of the `strings` package is appropriate for the new functionality that involves string manipulation (line 7). The `FlagSkipValidate` variable is introduced to allow users to skip configuration validation during migration (line 19). This could be useful when migrating unknown or custom configurations. * 68-79: The `Upgrade` function is called with the `plan` that is determined based on the `targetVersion` and `configType` (line 76). The `FlagSkipValidate` is passed to the `Upgrade` function, which allows skipping validation as per the user's choice. This is a good use of context to control the verbosity of the migration process (lines 68-69). The logic to determine the `outputPath` based on the `FlagStdOut` is also sound (lines 71-74). </blockquote></details></blockquote></details> </details>
|
Hi everyone, still waiting for your review. Cheers! |
| Use: "diff [target-version] <config-path> [config-type]", | ||
| Short: "Outputs all config values that are different from the default.", | ||
| Long: "This command compares the specified configuration file (app.toml or client.toml) with the defaults and outputs any differences.", | ||
| Args: cobra.MinimumNArgs(2), |
There was a problem hiding this comment.
Oh, this wasn't resolved. cobra.MinimumNArgs(2) should have stayed as 1, as the path shouldn't be always needed. I'll make a follow-up.
Description
Closes: #18435
Parameterized the migrate function to include
client.tomlfiles.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
confix migratecommand to support specifying configuration file types (clientorapp) for targeted migrations.Documentation
Bug Fixes
LoadLocalConfigfunction to prevent application panics and return errors gracefully.Refactor
DiffCommandandMigrateCommandfunctions to handle an additionalconfig-typeargument, improving the flexibility of configuration file comparisons and migrations.getFileNamefunction to construct filenames based on configuration type, streamlining the configuration loading process.Tests
MigrateCommandto include new scenarios and verbose output options.