fix(client): get desired config file #22261
Conversation
GAtom22
requested review from
julienrbrt,
JulianToledano and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the configuration handling logic in Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)
🔇 Additional comments (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
GAtom22
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/config/toml.go (2)
79-82: LGTM: Good use of constants, consider moving them.The addition of
confNameandconfTypeconstants improves code maintainability by eliminating magic strings. This aligns well with the Uber Go Style Guide.Consider moving these constants to the package level if they might be used elsewhere in the future. This would improve reusability without affecting the current implementation.
86-86: LGTM: Explicit config file setting addresses the core issue.The addition of
v.SetConfigFilewith a dynamically constructed path directly addresses the PR objectives. This should resolve the issue of incorrect configuration file selection mentioned in the PR summary.Consider using
filepath.Joininstead of string concatenation for better cross-platform compatibility:v.SetConfigFile(filepath.Join(configPath, fmt.Sprintf("%s.%s", confName, confType)))This would require adding
"path/filepath"to the imports.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- client/config/toml.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
client/config/toml.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
client/config/toml.go (2)
5-5: LGTM: Import addition is appropriate.The addition of the
fmtpackage import is consistent with the upcoming changes in thegetClientConfigfunction. This follows the Uber Go Style Guide recommendations for standard library imports.
84-85: LGTM: Improved consistency with constants.The use of
confNameandconfTypeconstants inSetConfigNameandSetConfigTypeimproves code consistency and maintainability. This change aligns with the PR objectives and the Uber Go Style Guide.
|
Could you add a test or at least steps to reproduce? I've never noticed an issue actually 🤔 |
Description
Closes: #XXXX
When calling the
conf, err := getClientConfig(configPath, ctx.Viper)on the client configuration, the current code calls the following functions on theViperinstance:However, if having a custom home directory specified, this will result in picking up an incorrect configuration.
This happens because the logic in place does the following:
v.AddConfigPath(configPath)adds the specified configPath to a slicev.SetConfigName("client")sets thev.configNameto the value provided, AND sets the v.configFile = "" THIS IS THE ISSUEv.SetConfigType("toml")does the same for thev.configTyoev.ReadInConfig()calls thegetConfigFile. This function checks forv.configFile == ""(WHICH is TRUE) and calls afindConfigFilefunction that searches for the config file in the slice of config paths.This logic can result in using an incorrect configuration file.
To ensure the desired configuration file is used (using the provided
configPath), this PR adds the call to theSetConfigFilefunction, which explicitly sets thev.configFilevalue.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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation