-
Notifications
You must be signed in to change notification settings - Fork 286
feat: Add TF samples for creating a Looker (Google Cloud core) Enterprise instance that uses PSC #871
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
feat: Add TF samples for creating a Looker (Google Cloud core) Enterprise instance that uses PSC #871
Conversation
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
|
Intending to use this snippet on the following page: |
glasnt
left a comment
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.
Generally this sample looks okay, but other samples in the looker folder have configurations to skip tests. It will probably be needed for this one.
Example: https://github.com/terraform-google-modules/terraform-docs-samples/blob/main/looker/looker_instance_basic/test.yaml, where metadata.name is the folder name and will need to be changed.
looker/looker-core-psc/main.tf
Outdated
| client_secret = "my-client-secret" | ||
| } | ||
| psc_config { | ||
| allowed_vpcs = ["projects/test-project/global/networks/test"] |
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 value won't work, as test-project is not a valid project ID.
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 updated the comments in the sample. The test-project and test parts of the VPC URL are unique to the user's project, so the value here is just an example.
looker/looker-core-psc/main.tf
Outdated
| # update only | ||
| # service_attachments = [{local_fqdn: "www.local-fqdn.com" target_service_attachment_uri: "projects/my-project/regions/us-east1/serviceAttachments/sa"}] |
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.
Are these comments needed?
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 updated the comments to hopefully be more useful!
|
/gcbrun |
Co-authored-by: Katie McLaughlin <[email protected]>
Co-authored-by: Katie McLaughlin <[email protected]>
|
@glasnt thanks for taking a look! I have hopefully addressed your comments. If it helps, I took the sample directly from what the SME engineer uploaded to https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/looker_instance#example-usage---looker-instance-psc. |
|
/gcbrun |
looker/looker-core-psc/main.tf
Outdated
| psc_config { | ||
| allowed_vpcs = ["projects/test-project/global/networks/test"] | ||
| # (Optional) List of VPCs that are allowed ingress into the Looker instance. | ||
| service_attachments = [{local_fqdn: "www.local-fqdn.com" target_service_attachment_uri: "projects/my-project/regions/us-east1/serviceAttachments/sa"}] |
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.
If I'm reading the docs correctly, you don't set these values, they are updated for you. Trying to validate this part of the Terraform is failing with An argument named "service_attachments" is not expected here.. Are you able to confirm this is intended as an argument?
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.
Ah, I get it! So the service attachment argument is relevant only when updating an instance config, not when creating it. I think that to match the other blocks in https://cloud.google.com/looker/docs/looker-core-create-psc#create_instance we should remove that argument entirely.
As for the allowed_vpcs, you would only specify that if you are creating an instance that uses only private IP (that is:
private_ip_enabled = true
public_ip_enabled = false
psc_enabled = true
This example doesn't follow that pattern, so I have commented out the allowed_vpcs and added some context.
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
…rise instance that uses PSC (terraform-google-modules#871) * feat: Prepared looker-core-psc for inclusion in C.G.C. documentation * Update correct copyright year Co-authored-by: Katie McLaughlin <[email protected]> * Use "default" rather than "main" Co-authored-by: Katie McLaughlin <[email protected]> * reviewer updates * add test.yaml file to skip tests * update metadata * clarify allowed vpc and service attachment arguments * lint * lint --------- Co-authored-by: Katie McLaughlin <[email protected]> Co-authored-by: Katie McLaughlin <[email protected]>
Description
Fixes b/429215192
Note: If you are not associated with Google, open an issue for discussion before submitting a pull request.
Checklist
Readiness
Style
guide
Testing
I have performed tests described in the Contributing guide:
terraform applyterraform fmtcheckIntended location
Yes, this sample will be (or already is) included on cloud.google.com
Location(s):
No, this sample won't be included on cloud.google.com
Reason:
API enablement
Review