#5851 Fix AttributeError on t.i.ssl.Certificate repr #11930
Conversation
for more information, see https://pre-commit.ci
|
Many thanks, Rob for the fix. For a PR to be merged the changes need to have a release note fragment and the changes to include the tests. More info about the release notes here Let us know if you need help with the release notes or writing the tests. Regards |
|
@rvandam I'm keen to get this merged do you need help writing a test for this change? |
|
That would be great. Haven’t had time to get back to it yet.
…On Sat, Sep 9, 2023 at 11:38 AM Thomas Grainger ***@***.***> wrote:
@rvandam <https://github.com/rvandam> I'm keen to get this merged do you
need help writing a test for this change?
—
Reply to this email directly, view it on GitHub
<#11930 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPELPBQEB736TNDCP3BPMLXZSSSXANCNFSM6AAAAAA35YX2XE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
graingert
changed the title
| c = sslverify.Certificate.loadPEM(ubuntuOneGoDaddyPem) | ||
| self.assertEqual( | ||
| repr(c), | ||
| "<Certificate Subject=b'Go Daddy Secure Certification Authority' Issuer=>", |
There was a problem hiding this comment.
might be better to omit the Issuer= entirely here?
There was a problem hiding this comment.
I think this is ok. no need to spend too much time with the repr part. this is only to help with the debugging.
|
@adiroiban I've added a test case to this PR |
There was a problem hiding this comment.
Thanks for the update.
Not sure what to say about ubuntu one cert.
It's good to have a certificate from real world, but for testing, I would prefer a certificate that is more focused on the scope of that particular test.
I would argue that a barebone certificate is better here.
I left a few comments inline.
Thanks for the update.
| c = sslverify.Certificate.loadPEM(ubuntuOneGoDaddyPem) | ||
| self.assertEqual( | ||
| repr(c), | ||
| "<Certificate Subject=b'Go Daddy Secure Certification Authority' Issuer=>", |
There was a problem hiding this comment.
I think this is ok. no need to spend too much time with the repr part. this is only to help with the debugging.
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
|
I have enabled auto-merge |
Scope and purpose
Fixes #5851