Skip to content

Tweak CSR parsing errors/documentation#390

Merged
djc merged 3 commits into
mainfrom
tweak-csr-errors
Nov 10, 2025
Merged

Tweak CSR parsing errors/documentation#390
djc merged 3 commits into
mainfrom
tweak-csr-errors

Conversation

@djc

@djc djc commented Oct 13, 2025

Copy link
Copy Markdown
Member

@djc djc requested review from cpu and est31 October 13, 2025 09:13
@djc djc mentioned this pull request Oct 13, 2025
Closed
cpu
cpu approved these changes Oct 14, 2025
View reviewed changes

@cpu cpu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One wording nit, but in sum LGTM. Thanks!

Comment thread rcgen/src/error.rs Outdated
@djc djc force-pushed the tweak-csr-errors branch from d992370 to 39e8a81 Compare October 14, 2025 14:18
@dwhjames

dwhjames commented Oct 15, 2025

Copy link
Copy Markdown
Contributor

Following up from:

Note, there are now three distinct ways that the context of a CSR is indicated in an error name:

rcgen/rcgen/src/error.rs

Lines 9 to 10 in 39e8a81

/// The given certificate signing request couldn't be parsed
CouldNotParseCertificationRequest,

rcgen/rcgen/src/error.rs

Lines 13 to 15 in 39e8a81

/// The CSR signature is invalid
#[cfg(feature = "x509-parser")]
InvalidRequestSignature,

rcgen/rcgen/src/error.rs

Lines 42 to 43 in 39e8a81

/// Unsupported field when generating a CSR
UnsupportedInCsr,

Probably other things to critique about Error::InvalidSignatureInCsr, but it does avoid creating a 3rd naming style.

@djc

djc commented Oct 15, 2025

Copy link
Copy Markdown
Member Author

That's fair. I've renamed InvalidRequestSignature to InvalidCertificationRequestSignature to alleviate that concern.

@djc djc added this pull request to the merge queue Nov 10, 2025
Merged via the queue into main with commit 6bbe775 Nov 10, 2025
15 of 17 checks passed
@djc djc deleted the tweak-csr-errors branch November 10, 2025 06:27
@dwhjames

dwhjames commented Nov 10, 2025

Copy link
Copy Markdown
Contributor

That's fair. I've renamed InvalidRequestSignature to InvalidCertificationRequestSignature to alleviate that concern.

@djc I think you forget to push that change?

@djc djc mentioned this pull request Nov 11, 2025
Merged
@djc

djc commented Nov 11, 2025

Copy link
Copy Markdown
Member Author

Huh, indeed -- thanks for pointing it out.

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

Reviewers

@cpu cpu cpu approved these changes

@est31 est31 est31 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@djc @dwhjames @cpu @est31