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

Fixes API breaking changes. #557

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

Superhepper
Copy link
Collaborator

This is an attempt to avoid breaking the API in major version 7.

ionut-arm
ionut-arm previously approved these changes Dec 3, 2024
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Looks good!

@Superhepper Superhepper force-pushed the 7.x.y-keep-api-intact branch from 3339b0e to bd3bbaa Compare December 4, 2024 09:11
@Superhepper Superhepper force-pushed the 7.x.y-keep-api-intact branch 2 times, most recently from 046f466 to 55fd6a0 Compare December 4, 2024 14:26
@Superhepper Superhepper marked this pull request as ready for review December 4, 2024 14:42
@Superhepper Superhepper requested a review from ionut-arm December 4, 2024 14:42
@Superhepper
Copy link
Collaborator Author

@THS-on Did you have the time to check if changes would have any effect on the RSA 2048 keys?

wiktor-k
wiktor-k previously approved these changes Dec 4, 2024
@@ -228,8 +230,36 @@ pub fn load_ak(
Ok(key_handle)
}

/// This creates an Attestation Key in the Endorsement hierarchy
/// This creates an Attestation Key in the Endorsement hierarchy.
pub fn create_ak<IKC: IntoKeyCustomization>(
Copy link
Collaborator

@wiktor-k wiktor-k Dec 4, 2024

Choose a reason for hiding this comment

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

I think it'd be good to mark one of them as #[deprecated] with a message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about that. The problem is that the function will not be removed but it will have its arguments changed in the next major version.

So xx will have its arguments changed.
and xx_2 will be removed. So xx_2 is the function that we want people to use right now. But it will be removed so marking it deprecated here could cause a lot of confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, so let's not do that. But a doc-comment WARNING note or something may still be a good idea? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is an excellent idea. I need to rephrase the comments any way because I could barely understand them my self when I tried to look at the code in the browser.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I have managed to fix it now. Please have a last look at the comments so I have not made some embarrassing mistakes @wiktor-k @ionut-arm

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it looks really nice 👌

Signed-off-by: Jesper Brynolf <[email protected]>
@Superhepper Superhepper force-pushed the 7.x.y-keep-api-intact branch from 0d7c815 to 5590896 Compare December 5, 2024 18:35
Copy link
Member

@ionut-arm ionut-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, thanks!!

@ionut-arm ionut-arm merged commit fac18ce into parallaxsecond:7.x.y Dec 6, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants