cli/config: improve handling of errors

thaJeztah commented 2 months ago

Improving some parts of how we handle failures when loading the CLI configuration file. This relates to https://github.com/docker/cli/issues/5075, but does not yet fix that issue (only handling some additional errors);

https://github.com/docker/cli/issues/5075

Changes in this PR should not likely cause a change in behavior (so could be included in a patch release); follow-up changes are still needed to handle various scenarios where we should hard-fail, but there's many different related code-paths, so this still needs work.

cli/config: Load(): improve GoDoc

cli/config: add test for dangling symlink for config-file

This may need further discussion, but we currently handle dangling symlinks gracefully, so let's add a test for this, and verify that we don't replace symlinks with a file.

cli/config: TestLoadDefaultConfigFile: check that no warnings are printed

Loading the config should print no warnings on a successful load.

cli/config: do not discard permission errors when loading config-file

When attempting to load a config-file that exists, but is not accessible for the current user, we should not discard the error.

This patch makes sure that the error is returned by Load(), but does not yet change LoadDefaultConfigFile, as this requires a change in signature.

- Description for the changelog

Print a warning when the CLI does not have permissions to read the configuration file.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah commented 2 months ago

Ah 🙈 probably testing won't work if it's running as root;

59.75 === FAIL: cli/config TestLoadNoPermissions (0.00s)
59.75     config_test.go:79: assertion failed: error is nil, not "permission denied" (os.ErrPermission)
59.75 
59.75 === FAIL: cli/config TestLoadDefaultConfigFile/permission_error (0.00s)
59.75     config_test.go:377: assertion failed: string "" does not contain "WARNING:"
59.75     config_test.go:378: assertion failed: string "" does not contain "permission denied"

codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 61.34%. Comparing base (57a1180) to head (c102e29).

:exclamation: Current head c102e29 differs from pull request most recent head dc75e9e

Please upload reports for the commit dc75e9e to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #5077 +/- ## ========================================== + Coverage 61.32% 61.34% +0.02% ========================================== Files 298 298 Lines 20706 20706 ========================================== + Hits 12698 12703 +5 + Misses 7106 7103 -3 + Partials 902 900 -2 ```

thaJeztah commented 2 months ago

@krissetto I made some changes; PTAL if it LGTY

krissetto commented 2 months ago

@krissetto I made some changes; PTAL if it LGTY

looking good @thaJeztah 😎

thaJeztah commented 2 months ago

Let me bring this one in; I'll try to spend some time on fixing the actual handling related to https://github.com/docker/cli/issues/5075, but taking it small steps at a time.

docker / cli