fix: avoid hard fail when no config json file is present#164
fix: avoid hard fail when no config json file is present#164KatIsCoding wants to merge 6 commits into
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!
|
|
||
| // Filepath returns the path to the docker cli config file, | ||
| // checking if the file exists. | ||
| var ErrConfigFileNotFound = errors.New("config file not found") |
There was a problem hiding this comment.
suggestion: could you locate this sentinel error in an errors.go file? Other packages/modules already do that. Also, the comment for the next function has been moved.
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! |
| cfg, err = loadFromFilepath(p) | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return cfg, nil | ||
| } |
| configFilePath := filepath.Join(dir, FileName) | ||
| if !fileExists(configFilePath) { | ||
| return "", fmt.Errorf("config file does not exist (%s)", configFilePath) | ||
| return "", errors.Join(fmt.Errorf("config file does not exist (%s)", configFilePath), ErrConfigFileNotFound) | ||
| } |
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.