-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
r/key_vault_certificate: exposing the certificate data
field
#1200
r/key_vault_certificate: exposing the certificate data
field
#1200
Conversation
c7b01af
to
6ac6706
Compare
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 @thomastaylor312
Thanks for this PR :)
I've taken a look through and this mostly LGTM - if we can fix up the potential crash this should be good to merge :)
Thanks!
@@ -339,6 +344,7 @@ func resourceArmKeyVaultCertificateRead(d *schema.ResourceData, meta interface{} | |||
// Computed | |||
d.Set("version", id.Version) | |||
d.Set("secret_id", cert.Sid) | |||
d.Set("certificate_data", string(*cert.Cer)) |
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.
there's a potential for a crash here, can we make this:
if contents := cert.Cer; contents != nil {
d.Set("certificate_data", string(*contents))
}
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.
I was wondering about that. It seemed like a certificate should always be set, but I'll put in a check.
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.
that's generally true - however the Azure API's return the state of a resource when it was last modified, rather than as it is right now (as such, it's possible it could be nil)
certificate data
field
6ac6706
to
16bc390
Compare
Ok, changes made! Thanks for the review @tombuildsstuff |
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 @thomastaylor312
Thanks for pushing the latest changes - this now LGTM, so I'll kick off the test suite now 👍
Thanks!
Also updating a couple of the older resource names to be correct
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
Every other key vault resource returns the data