cli icon indicating copy to clipboard operation
cli copied to clipboard

cli/config/credentials: refactor DetectDefaultStore and add tests

Open thaJeztah opened this issue 1 year ago • 3 comments

Refactor the DetectDefaultStore to allow testing it cross-platform, and without the actual helpers installed.

This also makes a small change in the logic for detecting the preferred helper. Previously, it only detected the "helper" binary ("pass"), but would fall back to using plain-text if the pass credentials-helper was not installed.

With this patch, it falls back to the platform default (secretservice), before falling back to using no credentials helper (and storing unencrypted).

- Description for the changelog



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

thaJeztah avatar Oct 21 '24 20:10 thaJeztah

Codecov Report

Attention: Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 59.67%. Comparing base (32ff200) to head (555aba2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5568      +/-   ##
==========================================
+ Coverage   59.60%   59.67%   +0.07%     
==========================================
  Files         345      343       -2     
  Lines       29103    29113      +10     
==========================================
+ Hits        17346    17373      +27     
+ Misses      10788    10770      -18     
- Partials      969      970       +1     

codecov-commenter avatar Oct 21 '24 20:10 codecov-commenter

Thanks for reviewing! I fixed the typo; will have a look at the other bits tomorrow.

I guess we could generalize the for a credentials helper to have a "prerequisites check" (for docker-credentials-pass that would be "check if the pass binary is found". Other helpers would then have an empty func for that.

Perhaps ultimately, the credentials-helpers themselves could have some check (docker-credential-foo --check), but not sure if we want to go that route 😅

thaJeztah avatar Oct 28 '24 22:10 thaJeztah

Perhaps ultimately, the credentials-helpers themselves could have some check (docker-credential-foo --check), but not sure if we want to go that route

Honestly that sounds like the most sane idea to me – the CLI can't be expected to account for all the different credential helpers and their requirements, they should do it themselves. Maybe even something like the metadata we have for plugins, credential-helpers could have a similar thing that would include if they are "ready" to be used or not, and some error message.

Or the API could just be exit_code == 0 means it's fine, otherwise it's not and it prints some error message.

laurazard avatar Oct 30 '24 10:10 laurazard