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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

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

Patch coverage: 71.15% and project coverage change: -2.60 ⚠️

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%) ⬇️

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

@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)

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

Added support for secretservice as well: #290

@crazy-max crazy-max marked this pull request as draft June 13, 2023 10:51
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
3 participants