-
Notifications
You must be signed in to change notification settings - Fork 104
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
add support email address #548
base: main
Are you sure you want to change the base?
Conversation
@austinvalle hello! Sorry to bother you, could you review this PR, this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @iishabakaev 👋🏻 , thanks for submitting the PR and apologies for the delay in review!
I have left some specific code feedback on the PR as well as I have some general feedback that will need to be addressed before we can accept the change:
- This PR only updates the
tls_cert_request
schema to include this new field, however the underlying logic is also shared bytls_self_signed_cert
.- I think we should adjust the
tls_self_signed_cert
schema as well to include this newemail_address
field so we can fully close Add support foremail_addresses
intls_cert_request
andtls_self_signed_cert
resources #35
- I think we should adjust the
- This PR will need acceptance tests before we can merge it. This should just be updating the existing ones that test the subject and just verifying the data is properly set:
- This PR will need a changelog, but I can also help with that once the PR is updated to address some of the feedback
f1532d4
to
89c3850
Compare
Some general housekeeping things:
|
Updated PR based on all comments For testing I had to tinker a bit, the standard function x509.ParseCertificate parses ExtraNames in raw, so I added the creation of email_address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @iishabakaev!
I've opened a mirror PR which we can eventually use to run the CI tests (I still need to manually move the changes/approve them 😄 )
#599
Currently I think the remaining items for this PR are:
- Run the doc generator locally to fix that diff in our CI
- Remove the changes to
docs/cdktf
- Verify the tests are all green ✅
After that I can push a changelog and merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is a test failure in the PR, see comment in review.
Also you can run the tests locally by just running make testacc
or just running with any go test
command equivalent with the env variable TF_ACC=1
@@ -586,6 +596,7 @@ func selfSignedCertConfig(validity, earlyRenewal uint32) string { | |||
country = "US" | |||
postal_code = "95559-1227" | |||
serial_number = "2" | |||
email_address = "[email protected]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You won't be able to add this here since tests like TestAccResourceSelfSignedCert_UpgradeFromVersion3_4_0
also use this function, which tests migration from an older version of the provider that doesn't have the email_address
attribute
https://github.com/hashicorp/terraform-provider-tls/actions/runs/12956793277/job/36143882711?pr=599
You can duplicate the config in TestResourceSelfSignedCert
to just add email address there, or make it a parameter in this function, whichever you prefer
7bac169
to
7e9ec22
Compare
Same as #405, because PR not merged since 2018
Fix this problem #35