cli/config/configfile: Atomically rewrite the config file when saving.#1359
Conversation
The config file was being truncated first, which created a window during which it was empty, causing concurrent uses of the `docker` command to potentially fail with: WARNING: Error loading config file: /var/lib/jenkins/.docker/config.json: EOF Error response from daemon: Get https://registry/v2/foo/manifests/latest: no basic auth credentials Signed-off-by: Benoit Sigoure <tsunanet@gmail.com>
|
Seems like others have run into this before me as well: https://issues.jenkins-ci.org/browse/JENKINS-44228 |
|
I for one would like to be the first to welcome our new atomic file writing overlords! |
|
The build failed due to some unrelated failure in an early |
Codecov Report
@@ Coverage Diff @@
## master #1359 +/- ##
==========================================
+ Coverage 54.63% 54.69% +0.05%
==========================================
Files 293 293
Lines 19369 19374 +5
==========================================
+ Hits 10582 10596 +14
+ Misses 8126 8111 -15
- Partials 661 667 +6 |
| assert.NilError(t, err) | ||
| cfg, err := ioutil.ReadFile("test-save") | ||
| assert.NilError(t, err) | ||
| assert.Check(t, is.Equal(string(cfg), "{\n \"auths\": {}\n}")) |
There was a problem hiding this comment.
Feels like an omitempty tag is missing in the Auths field.
There was a problem hiding this comment.
We can fix in another PR
There was a problem hiding this comment.
nit: Review after merge (so maybe for the just opened follow-up) 😅:
You can use backquotes instead:
assert.Check(t, is.Equal(string(cfg), `{
"auths": {}
}`))
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, thanks!
left one comment; I'll quickly check with @vdemeester if he thinks we should address before merging 👍
| os.Remove(temp.Name()) | ||
| return err | ||
| } | ||
| return os.Rename(temp.Name(), configFile.Filename) |
There was a problem hiding this comment.
nit; if the rename fails we'll leave behind the temp file.
Chances are probably slim that that happens, so I'm ok with keeping this as-is (we can do a follow-up if we think that's a concern)
There was a problem hiding this comment.
I feel it's not that big of a deal, so I'm ok keeping as-is and do a follow-up if we need 👼
There was a problem hiding this comment.
alrighty; let's get this merged; thanks!
There was a problem hiding this comment.
Excellent catch! Fixed in #1366 since this one already sailed.
8 participants
The config file was being truncated first, which created a window during
which it was empty, causing concurrent uses of the
dockercommand topotentially fail with:
Signed-off-by: Benoit Sigoure tsunanet@gmail.com
- Description for the changelog
The use of commands that change the Docker CLI config (e.g.
docker login) concurrently with other commands could lead to the other commands not being able to read the configuration.- A picture of a cute animal (not mandatory but encouraged)
🐹