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.

Sign up for GitHub

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

Jump to bottom

Replace references to Mbed Crypto (rebase) #7589

Merged
merged 3 commits into from
May 16, 2023
Merged

Replace references to Mbed Crypto (rebase) #7589

merged 3 commits into from
May 16, 2023

Conversation

daverodgman
Copy link
Contributor

@daverodgman daverodgman commented May 12, 2023 •
edited
Loading

Description

Rebased version of #4990.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

@daverodgman daverodgman mentioned this pull request May 12, 2023
Closed
@daverodgman
Copy link
Contributor Author

@AndrzejKurek since you reviewed previously, please could you TAL? @tom-daubney-arm - please could you also review, thanks

@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests historical-reviewing Currently reviewing (for legacy PR/issues) priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most) labels May 12, 2023
tom-cosgrove-arm
tom-cosgrove-arm approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tom-cosgrove-arm tom-cosgrove-arm added historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed historical-reviewing Currently reviewing (for legacy PR/issues) labels May 12, 2023
@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented May 12, 2023 •
edited
Loading

@daverodgman If you think one review is enough, you can add single-reviewer and approved, and remove needs-review. If you think it needs two reviews, then please add needs-reviewer

(I am on the fence here - it's at the edge of what I feel a single reviewer should approve)

@daverodgman daverodgman added approved Design and code approved - may be waiting for CI or backports single-reviewer This PR qualifies for having only one reviewer and removed needs-review Every commit must be reviewed by at least two team members, labels May 12, 2023
gilles-peskine-arm
gilles-peskine-arm approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (mostly, I checked that we do preserve the historical names in the storage specification)

@gilles-peskine-arm
Copy link
Contributor

This needs a 2.28 backport: Mbed TLS 2.28 is not a version of Mbed Crypto.

@gilles-peskine-arm gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label May 12, 2023
@gilles-peskine-arm
Copy link
Contributor

@tom-cosgrove-arm single-reviewer is up to the reviewer. The author should stay out of this decision due to an obvious conflict of interest. (I admit I have single-reviewered a PR of mine before, because the reviewers just weren't coming out and oking it even if they did approve. But it shouldn't be that way.)

@daverodgman
Copy link
Contributor Author

@tom-cosgrove-arm single-reviewer is up to the reviewer. The author should stay out of this decision due to an obvious conflict of interest. (I admit I have single-reviewered a PR of mine before, because the reviewers just weren't coming out and oking it even if they did approve. But it shouldn't be that way.)

It's fine for the author to mark a PR single-reviewer - it's more of a hint coming from the author. The reviewer can always remove that label if they feel a second reviewer is warranted, and the gatekeeper can do the same.

@daverodgman daverodgman removed the needs-ci Needs to pass CI tests label May 15, 2023
@daverodgman daverodgman mentioned this pull request May 15, 2023
Merged
3 tasks
@gilles-peskine-arm gilles-peskine-arm merged commit 63df4ec into Mbed-TLS:development May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@AndrzejKurek AndrzejKurek Awaiting requested review from AndrzejKurek

@tom-daubney-arm tom-daubney-arm Awaiting requested review from tom-daubney-arm

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports historical-reviewed Reviewed & agreed to keep legacy PR/issue needs-backports Backports are missing or are pending review and approval. priority-medium Medium priority - this can be reviewed as time permits single-reviewer This PR qualifies for having only one reviewer size-xs Estimated task size: extra small (a few hours at most)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daverodgman @tom-cosgrove-arm @gilles-peskine-arm @fredrikhesse