fix: avoid hard fail when no config json file is present#164
Conversation
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM, although I added a suggestion about the location of the new sentinel error. Also, could you unit test the new code paths?
Thanks for working on the fix!
There was a problem hiding this comment.
Pull request overview
This PR aims to make the SDK tolerant of missing Docker CLI config (~/.docker/config.json) by introducing a sentinel error and updating config/auth/context helpers to treat “no config file” as a valid empty/default state instead of a fatal error.
Changes:
- Add
config.ErrConfigFileNotFoundand include it inFilepath()errors to enableerrors.Is-based handling. - Update auth helpers (
AuthConfigs,AuthConfigForHostname) and context helpers (Current,New,Delete,store.inspect) to special-case the sentinel and avoid hard failures. - Make
Load()tolerateos.ErrNotExistfrom the read path (race between existence check and open).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
config/load.go |
Introduces ErrConfigFileNotFound, joins it into Filepath() errors, and relaxes Load() for os.ErrNotExist. |
config/auth.go |
Special-cases missing config file in auth entrypoints. |
context/current.go |
Falls back to DefaultContextName when Docker config is missing. |
context/context.add.go |
Treats missing config as a no-op when attempting to set the newly created context as current. |
context/context.delete.go |
Treats missing config as a no-op when attempting to reset current context on delete. |
context/store.go |
Treats missing config as a no-op during inspect (but currently skips required context field initialization). |
Comments suppressed due to low confidence (1)
config/load.go:84
- Even with
ErrConfigFileNotFound, a very common "fresh install" state is that the directory (~/.docker/DOCKER_CONFIG) is missing.Dir()currently errors when the directory is absent, and that error does not includeErrConfigFileNotFound, so callers checkingerrors.Is(err, ErrConfigFileNotFound)will still hard-fail. To fully address the reported scenario, the missing-directory case needs to surface the same sentinel (or otherwise become distinguishable from real I/O/permission errors).
func Filepath() (string, error) {
dir, err := Dir()
if err != nil {
return "", fmt.Errorf("config dir: %w", err)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ning, even when the config file is not found
|
@mdelapenya Thanks for your review! I implemented your suggestion, as well as some of the suggestions that copilot gave and the unit tests for the no config scenario! |
… and return the sentinel error on those too
… present when it is the homedir that is missing
… only when the config path has not been overwritten
|
@mdelapenya I updated some of the code paths and now I think they make more sense, I believe that on a fresh docker install, the default Let me know what you think! |
Closes: #149
Problem
Callers of
config.Load(),config.AuthConfigs,config.AuthConfigForHostname, and thecontextpackage helpers (Current,New,Delete,store.inspect) hard-fail when~/.docker/config.jsondoes not exist, even though the absence of a config file is a perfectly valid state on a fresh Docker installation.Root Cause
config.Filepath()returns a genericfmt.Errorf("config file does not exist (%s)", configFilePath)when the file is missing.Load()wraps that into"config path: ..."and propagates it upward. Because every caller treats any non-nil error as fatal, a missing config file tears down operations that should legitimately proceed with empty/default auth or the default context.Fix
Introduce a sentinel error,
config.ErrConfigFileNotFound, joined into the error returned byFilepath()viaerrors.Join, so callers can distinguish "no config file" from real I/O or parse failures usingerrors.Is.config/auth.go:AuthConfigsandAuthConfigForHostnamereturn an empty auth config when the file is missing, instead of erroring.context/current.go:Current()falls back toDefaultContextName(mirroring the existingos.IsNotExistbranch for the meta file).context/context.add.go,context/context.delete.go,context/store.go: the "set current" / "reset current" /inspectpaths treat a missing config file as a no-op rather than failing.config/load.go:Load()also toleratesos.ErrNotExistfromloadFromFilepath(race betweenFilepath()and the read) by returning an emptyConfig.Testing
configandcontextsuites continue to pass.