refactor: Generalize csr/crl signed_by to take &impl AsRef issuer#312
Conversation
This makes these consistent with Certificate::signed_by.
| self, | ||
| public_key: &impl PublicKeyData, | ||
| issuer: &impl AsRef<Self>, | ||
| issuer: &impl AsRef<CertificateParams>, |
There was a problem hiding this comment.
Hmm, I tried to make an explanation of this change in the PR summary.
There was a problem hiding this comment.
The argument you made seems reasonable to me in absence of a reason to prefer keeping it as-is.
|
Since we discovered this is a breaking change (#324) and reverted 0.13.3 we need to think about a path forward. We could:
I'm not sure I have a strong opinion. Initially I was thinking option 2, but perhaps it's best to wait until we have more breaking changes before incrementing that version component to avoid downstream churn? @djc @est31 @audunhalland Do you folks have thoughts? |
|
So #324 was about Then #307 is similarly a breaking change, it is not that different from this PR. I think it's better to have a more generic API than a more restricted one, so the change itself makes sense. But looking at the releases page, the 0.13 release is from December 2024, so from three months ago. Making another breaking change now would be a little bit fast I think. In the past I would have been "who cares, make a release", but rcgen has grown in size, and most of our downloads are still in the "other" category from before |
|
I consider Ideally I'd prefer not having The other option that works now and is backwards compatible is of course to add another set of methods: |
|
I don't like the idea of releasing 0.14 only for this change much -- too soon after the previous semver-incompatible release, for too little gain. @audunhalland if you'd like this API to become available sooner, adding it under a different method name could make sense for the 0.13.x line. I also want to spend more time cleaning up the issuer API to make it clearer/safer for this scenario of parsing an existing CA, and when that happens the simple change here might not make sense anymore. |
|
It sounds like we all agree that we don't want to make a 0.14 just for this change. I propose we revert the two (?) Sounds good? |
|
Reverting those two changes is fine by me, I can prepare another PR with alternative methods. |
Completes #307 by applying the same treatment to
csrandcrl.Also improves
CertificateParams::signed_byissuer fromAsRef<Self>toAsRef<CertificateParams>, because I think it's irrelevant that the issuer is the same type as the CertificateParams being signed, thusSelfshould not be used.