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

chore: Improve document about certificate #1715

Closed

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented May 29, 2024

Motivation

Adds documents related to #1707 separated from its breaking changes.

Solution

Adds some documents of certificates for that users may be confused about.

@tottoto tottoto changed the title Improve document about certificate chore: Improve document about certificate May 29, 2024
@@ -256,6 +256,12 @@ impl<T> Request<T> {
/// and is mostly used for mTLS. This currently only returns
/// `Some` on the server side of the `transport` server with
/// TLS enabled connections.
///
/// ## NOTE
/// Alghouth the returned ['Certificate'] is supposed to contain PEM
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "although"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Fixed it.

Comment on lines 20 to 21
/// `pem` must contain valid PEM encoded certificate. It is undefined
/// behavior to call this with `pem` that is not valid PEM encoded
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really undefined behavior? When I hear "UB" I think things like "the compiler is free to do whatever it wants", but in this case it seems like the behavior is pretty clear: You get a valid (in the rust sense) Certificate object containing invalid data (in the business logic sense). Am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Makes sense. The "UB" should be in the meaning of Rust. I updated it just a detailed document. By the way, now that I think about it, the constructor name is from_pem so it might not as confusing as I thought it would be. If this note section seems unnecessary, I will remove it.

@tottoto tottoto force-pushed the improve-document-about-certificate branch from 442dcdf to abdb5db Compare May 29, 2024 22:29
@djc
Copy link
Contributor

djc commented May 30, 2024

I don't think we'll want to merge this, we should merge (something like) #1707 instead.

@tottoto
Copy link
Collaborator Author

tottoto commented May 30, 2024

I understand. I will close this.

@tottoto tottoto closed this May 30, 2024
@tottoto tottoto deleted the improve-document-about-certificate branch June 15, 2024 09:25
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