cli/config: prevent warning if HOME is not set #2934
Conversation
thaJeztah
force-pushed
the
fix_homedir_warning
branch
from
259e57e
to
343cb43
Compare
thaJeztah
changed the title
|
@thaJeztah some tests are failing: |
Hmmm... ah, I suspect those go wrong because it will only fetch the home-directory once, and the test is updating the path 🤔. Will have to have a look how to solve that test |
commit c2626a8 replaced the use of github.com/docker/docker/pkg/homedir with Golang's os.UserHomeDir(). This change was partially reverted in 7a279af to account for situations where `$HOME` is not set. In situations where no configuration file is present in `~/.config/`, the CLI falls back to looking for the (deprecated) `~/.dockercfg` configuration file, which was still using `os.UserHomeDir()`, which produces an error/warning if `$HOME` is not set. This patch introduces a helper function and a global variable to get the user's home-directory. The global variable is used to prevent repeatedly looking up the user's information (which, depending on the setup can be a costly operation). Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
fix_homedir_warning
branch
from
343cb43
to
c85a37d
Compare
|
Fixed the tests, but it's a bit ugly; not sure if there's really a cleaner way though |
Codecov Report
@@ Coverage Diff @@
## master #2934 +/- ##
==========================================
+ Coverage 57.04% 57.05% +0.01%
==========================================
Files 297 297
Lines 18655 18656 +1
==========================================
+ Hits 10641 10644 +3
+ Misses 7154 7153 -1
+ Partials 860 859 -1 |
| ) | ||
|
|
||
| // resetHomeDir is used in testing to resets the "homeDir" package variable to | ||
| // force re-lookup of the home directory between tests. | ||
| func resetHomeDir() { |
There was a problem hiding this comment.
Maybe we can move this function in the tests files? I mean, tests are in the same package, so they have access to the homeDir variable.
There was a problem hiding this comment.
Yes, I was on the fence what would be better; I put it in the non-test file to make it more visible (I really don't like having to do this 😞)
| @@ -115,6 +115,7 @@ password`: "Invalid Auth config file", | |||
| email`: "Invalid auth configuration file", | |||
| } | |||
|
|
|||
| resetHomeDir() | |||
There was a problem hiding this comment.
I'm a bit on the fence between calling this function for each test as first step, to make sure the path is clean for the test, OR defer it at first step, to make sure everything will be cleared for other tests. 🤔
func TestOldInvalidsAuth(t *testing.T) {
defer resetHomeDir()
...
}func TestOldInvalidsAuth(t *testing.T) {
resetHomeDir()
...
}There was a problem hiding this comment.
It's tricky; any test that uses the homeDir() will initialise the package variable; my current approach was to put it in those tests that expect the variable to be re-resolved (because they patch the HOME variable for the test); changing the HOME variable is something that would only happen in tests (under normal circumstances it would not modify after the cli binary is started, so it would be safe to store it in the variable.
fixes moby/moby#41890 WARNING: Error loading config file: .dockercfg: $HOME is not defined
commit c2626a8 (#2101) replaced the use of github.com/docker/docker/pkg/homedir with Golang's
os.UserHomeDir().This change was partially reverted in 7a279af (#2111) to account for situations where
$HOMEis not set.In situations where no configuration file is present in
~/.config/, the CLI falls back to looking for the (deprecated)~/.dockercfgconfiguration file, which was still usingos.UserHomeDir(), which produces an error/warning if$HOMEis not set.This patch introduces a helper function and a global variable to get the user's home-directory. The global variable is used to prevent repeatedly looking up the user's information (which, depending on the setup can be a costly operation).
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)