-
-
Notifications
You must be signed in to change notification settings - Fork 373
ref: Use CryptoKit instead of CommonCrypto in SentryDsn #7037
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
Conversation
|
|
||
| ### Breaking Changes | ||
|
|
||
| - Bumped minimum macOS version from 10.14.0 to 10.15.0 |
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.
h: We can not do this due to #6758
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.
Thank you for the feedback. Given that the CryptoKit API is not available I suggest to close the PR. I've also added the Blocked label
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.
Can't we use it for newer macOS versions?
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.
I guess we can conditionally use CryptoKit and fallback to CommonCrypto when not available.
I think this would make things more complicated though and beat the purpose of using a simpler Swift friendly syntax.
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.
If there is not clear advantage to conditionally use CryptoKit let's not do this change.
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.
Sounds good 👍
I'm closing this PR. We can revisit it when bumping macos is possible.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7037 +/- ##
=============================================
- Coverage 85.027% 84.690% -0.337%
=============================================
Files 454 449 -5
Lines 27737 27415 -322
Branches 12159 12003 -156
=============================================
- Hits 23584 23218 -366
- Misses 4107 4150 +43
- Partials 46 47 +1
... and 71 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
📜 Description
CryptoKitinstead ofCommonCryptoinSentryDsn10.14to10.15that theInsecure.SHA1from CryptoKit requires💡 Motivation and Context
See #6942 (comment)
This is a swift improvement as a follow up to the above review feedback. It is not required thus feel free to reject/close the PR if the (breaking) change is not desired.
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.#skip-changelog
Closes #7038