mtrmac commented 2 years ago

Per https://github.com/containers/podman/issues/11636 , any errors reading specific credentials should be accompanied with an identification of that credential.

(Also check if we need to do that for any credential helper errors; credentials.NewErrCredentialsNotFound does not include any context.)

vrothberg commented 2 years ago

@mtrmac, I think that we can close this issue since https://github.com/containers/image/pull/1427 has merged. But maybe I am missing the point.

mtrmac commented 2 years ago

1427 adds special handling for a specific error; the proposal here not to change handling of specific errors, but to add context (which helper / file, which credential key) — especially for the unexpected errors we don’t handle specially.

E.g. if getAuthFromCredHelper fails with an unknown error, AFAICS we just return that all the way up to the caller, without saying which helper and which entry has failed in the returned error. (Some of that can be figured out from the debug log, but that’s only useful if the failure is reproducible.) Ideally this should be a subpackage-wide audit, not just affecting that one call.

mtrmac commented 2 years ago

E.g. in the Podman bug Error: 1 error occurred: … * credentials not found in native keychain is basically unactionable.

containers / image

pkg/docker/config.GetAllCredentials should add more context to errors #1406

1427 adds special handling for a specific error; the proposal here not to change handling of specific errors, but to add context (which helper / file, which credential key) — especially for the unexpected errors we don’t handle specially.