Fix issue where one bad credential helper causes no credentials to be returned

[laurazard]

Signed-off-by: Laura Brehm [email protected]

fixes #3716

- What I did

Instead of returning no credentials when a configured credential helper throws an error, skip the bad credential helper, return the remaining credentials, and warn the user about the error.

- How to verify it

Refer to the reproduction steps in #3716, or run the added unit tests 😄

- Description for the changelog

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

[codecov-commenter]

Codecov Report

Merging #3996 (7554eda) into master (4a500f6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3996      +/-   ##
==========================================
+ Coverage   59.20%   59.21%   +0.01%     
==========================================
  Files         287      287              
  Lines       24698    24698              
==========================================
+ Hits        14622    14625       +3     
+ Misses       9192     9190       -2     
+ Partials      884      883       -1     

[thaJeztah]

Was looking at this again, and while we should look at using the logger more, we should probably look at the output for it. Currently, the output looks like;

WARN[0000] Failed to get credentials for registry: foo   error="something went wrong"

Still readable, but perhaps not as "friendly". I see elsewhere we just print our own, e.g.;

cli/cli/config/config.go

Line 144 in 3ae101f

fmt.Fprintf(stderr, "WARNING: Error loading config file: %v\n", err)

Which may look like;

WARNING: Error loading config file: /root/.docker/config.json: json: cannot unmarshal string into Go struct field ConfigFile.auths of type types.AuthConfig

Only issue is that this location of the code currently doesn't have a reference to the CLI's stdout or stderr, so if we change it, we probably need to hardcode it to os.Stderr (not ideal either).

[thaJeztah]

Ideas / suggestions (perhaps someone else has thoughts) welcome.

Don't think it's necessarily a blocker, if there's no better solution (for now)

[laurazard]

That's a good point! In general I think having the more standardized logging is good (vs just calling fmt.Fprintf), but the friendliness is a good point. Maybe we can look into seeing if we can configure the output for it. I don't have better ideas right now though 😅

[thaJeztah]

Yes, there's a lot to look into in this area;

• There's fmt.PrintXX in many places
• While it's "generally" ok to use the defaults (I suspect logrus is not configured anywhere to use something other than os.StdOut / os.StdErr), ideally we'd have better controls for that
• ^^ sometimes we need to use a different approach depending on (e.g.) if an actual TTY is attached (see cli: additionalHelp() don't decorate output if it's piped, and add extra newline #3973)
• ^^ or (e.g.) if we want the output to be "quiet" (could be in the CLI itself, or other projects using parts of the CLI as a "library")
• I also went looking for some of that, e.g. WIP: make printing output easier (approach 1) #3797
• (as mentioned) logrus (or other loggers) are great for logs, but the default output format often isn't great for printing "one-off", user-facing messages

So yes, this needs work (perhaps I should create a tracking ticket somewhere); e.g.

• Consider using a context-logger to pass a logger around
• That logger could (default) be a logger with a format that's "friendly enough"
• Look into best approach to pass around the streams (STDIN/STDOUT/STDERR), but when doing so, try to prevent coupling things too tightly (i.e., we may not want to pass cobra.Command everywhere).

[thaJeztah]

LGTM, thanks!

Let's get this one in; we can bike-shed on the output format and refactoring later (most users (hopefully) wouldn't see this message).