Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented May 27, 2023

closes #280

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2023

Codecov Report

Attention: Patch coverage is 71.15385% with 15 lines in your changes missing coverage. Please review.

Project coverage is 50.54%. Comparing base (8438667) to head (ffe5a98).

Files with missing lines Patch % Lines
osxkeychain/osxkeychain.go 71.15% 8 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
- Coverage   53.10%   50.54%   -2.57%     
==========================================
  Files           9        9              
  Lines         676      645      -31     
==========================================
- Hits          359      326      -33     
- Misses        272      273       +1     
- Partials       45       46       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@crazy-max crazy-max force-pushed the darwin-go-keychain branch 10 times, most recently from 82b3d3e to a4d4911 Compare May 27, 2023 22:51
@crazy-max crazy-max force-pushed the darwin-go-keychain branch 3 times, most recently from 72398ea to f559c41 Compare May 28, 2023 16:18
@crazy-max crazy-max marked this pull request as ready for review May 28, 2023 16:19
@thaJeztah
Copy link
Member

Haven't checked yet, but possibly this also fixes #177

@thaJeztah
Copy link
Member

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 #268, #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)

@crazy-max
Copy link
Member Author

Added support for secretservice as well: #290

@crazy-max crazy-max marked this pull request as draft June 13, 2023 10:51
Copy link
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably start testing it on macOS 15. Otherwise code LGTM.

@akerouanton
Copy link
Member

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 other back-ends

It didn't receive any new backend implementation since this PR was opened. So this should clear out any concerns about it becoming bloated (from our point-of-view).

It seems to be in 'maintenance mode' but given that it supports a limited number of backends, and these tend to be stable by nature I guess that's a good thing.

We can still decide to check out / fork it if we have any problems in the future, but for I now I guess it's okay to pull that depedency in.

@thaJeztah I think your other comments are not actionable at this point (or at least not in this PR). Do you have any other concerns, or can we merge it as is?

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
Copy link
Member Author

rebased after #357 merged

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit a7974e9 into docker:master Feb 27, 2025
12 checks passed
@crazy-max crazy-max deleted the darwin-go-keychain branch February 27, 2025 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[chore] update osxkeychain implementation to account for deprecated SDK functions
4 participants