docker-credential-helpers icon indicating copy to clipboard operation
docker-credential-helpers copied to clipboard

osxkeychain: switch to github.com/keybase/go-keychain

Open crazy-max opened this issue 2 years ago • 4 comments

closes https://github.com/docker/docker-credential-helpers/issues/280

crazy-max avatar May 27 '23 21:05 crazy-max

Codecov Report

Patch coverage: 71.15% and project coverage change: -2.60 :warning:

Comparison is base (83d38ea) 52.74% compared to head (e61a226) 50.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
- Coverage   52.74%   50.15%   -2.60%     
==========================================
  Files           9        9              
  Lines         673      642      -31     
==========================================
- Hits          355      322      -33     
- Misses        271      272       +1     
- Partials       47       48       +1     
Impacted Files Coverage Δ
osxkeychain/osxkeychain.go 56.52% <71.15%> (-12.59%) :arrow_down:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar May 27 '23 21:05 codecov-commenter

Haven't checked yet, but possibly this also fixes https://github.com/docker/docker-credential-helpers/issues/177

thaJeztah avatar May 28 '23 16:05 thaJeztah

So the only concerns I have is that this

  • adds a new dependency (it looks actively maintained, but doesn't have tagged releases yet)
  • do we know what the scope is of the dependency? you mentioned secretservice, and it looks indeed that they implemented a package for that as well in https://github.com/keybase/go-keychain/tree/c2ce0606900517e98f5dbfbd7d03f91424bc5867/secretservice
  • ^^ that can be both a "positive" (perhaps it's an implementation we could also use), but could also be a "negative" if they start adding <unlimited> other back-ends

In general, I'm wondering if we should start to reorganise this repository into multiple modules, because it's now a "mono-repo" containing both the client / API / library code (client, credentials, registryurl), as well as actual implementations (wincreds, osxkeychain, secretservice, pass), with (possibly) more implementations to be added (see https://github.com/docker/docker-credential-helpers/pull/268, https://github.com/docker/docker-credential-helpers/pull/235)

Some possible approaches;

  • make each implementation its own module (which can also be tagged separately)
  • somehow refactor the client / API / library code to become its own module (and tag that separately)
  • ^^ using vendoring would probably be a bit of a challenge though (vendoring usually would be for binary code, which would now potentially be spread over multiple locations, unless we have a implementation/ or cmd/ directory shared between implementations)

thaJeztah avatar May 28 '23 16:05 thaJeztah

  • do we know what the scope is of the dependency? you mentioned secretservice, and it looks indeed that they implemented a package for that as well in https://github.com/keybase/go-keychain/tree/c2ce0606900517e98f5dbfbd7d03f91424bc5867/secretservice

  • ^^ that can be both a "positive" (perhaps it's an implementation we could also use), but could also be a "negative" if they start adding <unlimited> other back-ends

Added support for secretservice as well: https://github.com/docker/docker-credential-helpers/pull/290

crazy-max avatar Jun 13 '23 10:06 crazy-max