-
Notifications
You must be signed in to change notification settings - Fork 1.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
Terraform Data Source to retrieve SQL instance CA certs #2901
Terraform Data Source to retrieve SQL instance CA certs #2901
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs. Thanks for your contribution! A human will be with you soon. @danawillow, please review this PR or find an appropriate assignee. |
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 @chrissng!
}, | ||
Computed: true, | ||
}, | ||
"instance": { |
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.
Instead of separate instance
and instance_self_link
, let's have just one field that can take the instance name or self link. You can do this by using the compareSelfLinkOrResourceName
DiffSuppressFunc
- the logic you already have for the project field value will take care of getting the correct project.
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.
This is great as it simplifies the implementation. Done.
[API](https://cloud.google.com/sql/docs/mysql/admin-api/rest/v1beta4/instances/listServerCas). | ||
|
||
|
||
## Example Usage |
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.
Is there a terraform-relevant example of how this might be used, such as another resource that might interpolate on this value? If so, mind adding it as well?
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 am not able to find a TPG resource that could be interpolated into; typically this is used by applications to connect to the db via ssl. Therefore such a usage would be encapsulated as a Terraform module and exposed via relevant outputs.
I have extended my example a bit. Please see if it makes sense.
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 good!
|
||
The following attributes are exported: | ||
|
||
* `active_version` - The boot disk for the instance. Structure is documented below. |
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.
This description looks like a copy/paste error
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.
Oops my mistake. fixed.
return &schema.Resource{ | ||
Read: dataSourceGoogleSQLCaCertsRead, | ||
|
||
Schema: map[string]*schema.Schema{ |
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.
nit: Can you order this such that the fields go in the order Required -> Optional -> Computed?
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.
Done
config := meta.(*Config) | ||
|
||
var project, instance string | ||
if v, ok := d.GetOk("instance"); ok { |
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.
nit: instance is required, so we can remove this if/else and just keep the logic that's inside
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.
Good point. Removed
[API](https://cloud.google.com/sql/docs/mysql/admin-api/rest/v1beta4/instances/listServerCas). | ||
|
||
|
||
## Example Usage |
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 good!
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-google-conversion. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Gah I just merged the downstreams so don't do it in this PR, but can you create a followup to add a sidebar entry at https://github.com/GoogleCloudPlatform/magic-modules/blob/master/third_party/terraform/website-compiled/google.erb? |
- also extend example
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
@danawillow oh no I thought that was autogenerated. I'll raise a PR for the side bar asap |
Terraform Data Source to get all of the trusted Certificate Authorities (CAs) for the specified SQL database instance.
Example usage: This data source could be used to look for upcoming CA certs (as part of cert rotation) and if so, generate new client certs.
Release Note Template for Downstream PRs (will be copied)