Fix: Warning Message and Fallback search #1140
Conversation
There was a problem hiding this comment.
Thank you @Vimal-Raghubir for this PR!
LGTM, but can you update update your commit message with a sign-off as described here ?
cli/config/config.go
Outdated
| @@ -90,18 +90,18 @@ func Load(configDir string) (*configfile.ConfigFile, error) { | |||
| } | |||
|
|
|||
| // Can't find latest config file so check for the old one | |||
| confFile := filepath.Join(homedir.Get(), oldConfigfile) | |||
| confFile := filepath.Join(configDir, oldConfigfile) | |||
There was a problem hiding this comment.
Not sure this is actually correct; custom locations for the configuration file were added in moby/moby#12856, but at that time, .dockercfg was already the old file, and replaced with ~/.docker/config.json (that change was made in moby/moby#12009)
If we change this now, we're effectively adding new functionality (don't think setting --config-dir=/foo would read /foo/.dockercfg in older versions, correct? For a deprecated feature.
I think we should print a deprecation warning if we encounter an ~/.dockercfg file, and remove the logic to look for this filename in the next release (or the one after); this needs to be added to the deprecated features document; https://github.com/docker/cli/blob/master/docs/deprecated.md
There was a problem hiding this comment.
I do agree with @thaJeztah on this. This part of the code is looking for older/deprecated configuration file that was hardcoded in $HOME/.dockercfg… So we shouldn't change that (otherwise we're effectively adding a new functionnality).
There was a problem hiding this comment.
Same as @thaJeztah, I think we should deprecate and remove the code in later releases.
cli/config/config.go
Outdated
| @@ -90,18 +90,18 @@ func Load(configDir string) (*configfile.ConfigFile, error) { | |||
| } | |||
|
|
|||
| // Can't find latest config file so check for the old one | |||
| confFile := filepath.Join(homedir.Get(), oldConfigfile) | |||
| confFile := filepath.Join(configDir, oldConfigfile) | |||
There was a problem hiding this comment.
I do agree with @thaJeztah on this. This part of the code is looking for older/deprecated configuration file that was hardcoded in $HOME/.dockercfg… So we shouldn't change that (otherwise we're effectively adding a new functionnality).
|
Thank you @thaJeztah and @vdemeester for your review and feedback! So based on my understanding to your comments, this file path search has to be deprecated and removed eventually? The reason why I am asking this is I would love to help with the deprecation and removal of this portion if you wish to explore it anytime soon. |
|
Would I need to run the commands shown above by @GordonTheTurtle then run the squash commands? I small bit of help with this would be much appreciated. Thank you. |
|
Whoops, sorry, missed your ping; so to squash the commits, you do an "interactive rebase"; when doing an interactive rebase, you can mark commits to be "squashed" together. Here are some instructions that may help; https://docs.docker.com/v1.12/opensource/workflow/work-issue/#/pull-and-rebase-frequently. Make sure that the commit after it's been squashed is signed-off as well; if you forgot to do so, you can amend your commit with If all is done, you need to force push your branch (needed, because you did a rebase), which will update the pull request |
Signed-off-by: Vimal-Raghubir <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
rebased and squashed; @vdemeester PTAL |
|
Hi @thaJeztah, |
- What I did
fixes: #1110 . I removed the usage of the
$HOMEenvironment variable when the CLI searches for the.dockercfgfile and changed it to use the$DOCKER_CONFIGvariable as it should be doing in the first place.Next I changed the return statements for the error messages to display the new file path which is represented by the variable
filename, instead of the old config file path indicated byconfFile.- How I did it
I scanned the function calls and noticed the usage of
homedir.Get()for the$HOMEvariable. This was easy to realize that this has to be changed to the$DOCKER_CONFIGvariable. After that I scanned the error messages that used this old file path and realized confFile renders the old file path. This had to be replaced with filename which represents the new file path.- How to verify it
Verifying this can be done by rerunning the steps to reproduce the initial issue linked earlier. The warning message should be changed as well as the search should be done using the
$DOCKER_CONFIGvariable instead of the$HOMEvariable.- Description for the changelog
Fix warning message and ensure fallback search uses the DOCKER_CONFIG variable.
- A picture of a cute animal (not mandatory but encouraged)
