-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(client): get desired config file #22261
base: main
Are you sure you want to change the base?
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the configuration handling logic in Changes
Suggested reviewers
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
|
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.
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
confName
andconfType
constants 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.SetConfigFile
with 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.Join
instead 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
fmt
package import is consistent with the upcoming changes in thegetClientConfig
function. This follows the Uber Go Style Guide recommendations for standard library imports.
84-85
: LGTM: Improved consistency with constants.The use of
confName
andconfType
constants inSetConfigName
andSetConfigType
improves 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 🤔 |
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.
pre approve here, maybe add some tests in
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.
Could you add a test or at least steps to reproduce?
I've never noticed an issue actually 🤔
cc @GAtom22
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
Closes: #XXXX
When calling the
conf, err := getClientConfig(configPath, ctx.Viper)
on the client configuration, the current code calls the following functions on theViper
instance: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.configName
to the value provided, AND sets the v.configFile = "" THIS IS THE ISSUEv.SetConfigType("toml")
does the same for thev.configTyoe
v.ReadInConfig()
calls thegetConfigFile
. This function checks forv.configFile == ""
(WHICH is TRUE) and calls afindConfigFile
function 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 theSetConfigFile
function, which explicitly sets thev.configFile
value.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.md
Reviewers 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