-
Notifications
You must be signed in to change notification settings - Fork 26
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
Remove SAMKeychain dependency and use internal solution #1067
Conversation
Relates to #949 |
This is great! I'm actually in the middle of migrating dependencies for Tuple right now and these changes would be super handy. Looking forward to getting this downstream 👍 |
2b5b1eb
to
03494b7
Compare
Hmm, some to unpick here. I hoped this would be simple but @ricardopereira had forked from his Travis PR which is still in Draft. I'm going to rebase to remove the conflating commits, as I believe the SAMKeychain removal piece is pretty self-contained. |
03494b7
to
6783968
Compare
That's better. Essence restored. Sorry for the noise, @paddybyers and @tcard, but all good for review now. 😁 |
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 in principle, but I've not reviewed the code in detail.
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.
Sorry I'm late. I'm not really familiar with the involved APIs, but assuming their usage is correct, LGTM.
SAMKeychain has been archived and not maintained anymore #897.