-
Notifications
You must be signed in to change notification settings - Fork 182
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
82b3d3e
to
a4d4911
Compare
72398ea
to
f559c41
Compare
Haven't checked yet, but possibly this also fixes #177 |
So the only concerns I have is that this
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 ( Some possible approaches;
|
f559c41
to
e61a226
Compare
Added support for secretservice as well: #290 |
There was a problem hiding this 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.
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? |
e61a226
to
674beae
Compare
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>
674beae
to
ffe5a98
Compare
rebased after #357 merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
closes #280