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

Remove SAMKeychain dependency and use internal solution #1067

Merged
merged 2 commits into from
Oct 6, 2020

Conversation

ricardopereira
Copy link
Contributor

SAMKeychain has been archived and not maintained anymore #897.

@QuintinWillison
Copy link
Contributor

Relates to #949

@mikepulaski
Copy link
Contributor

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 👍

@QuintinWillison QuintinWillison self-assigned this Oct 5, 2020
@QuintinWillison QuintinWillison changed the base branch from travis-update-xcode to feature/remove-kscrash October 5, 2020 14:03
@QuintinWillison
Copy link
Contributor

QuintinWillison commented Oct 5, 2020

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.

@QuintinWillison
Copy link
Contributor

That's better. Essence restored.

Sorry for the noise, @paddybyers and @tcard, but all good for review now. 😁

Copy link
Member

@paddybyers paddybyers left a 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.

Base automatically changed from feature/remove-kscrash to main October 6, 2020 08:33
@QuintinWillison QuintinWillison merged commit 5d8a639 into main Oct 6, 2020
@QuintinWillison QuintinWillison deleted the remove-SAMKeychain-dep branch October 6, 2020 08:35
Copy link
Contributor

@tcard tcard left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants