-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Support subnetwork flow logs. #1385
Conversation
Add the `enable_flow_logs` field to our subnetwork resource, so we can specify whether [flow logs][1] should be enabled in Terraform configs. Note that this behavior isn't explicitly documented yet, but it has made it into the beta API client. [1]: https://cloud.google.com/vpc/docs/using-flow-logs
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 @paddycarver, these look good but I'm just concerned about what happens when we call the beta over the over the released api
} | ||
|
||
log.Printf("[DEBUG] Subnetwork insert request: %#v", subnetwork) | ||
|
||
op, err := config.clientCompute.Subnetworks.Insert(project, region, subnetwork).Do() | ||
subnetworkV1 := &compute.Subnetwork{} |
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.
What're your thoughts on moving this struct creation into here as well as everything specific to subnetworkV1?
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.
Was that supposed to link to https://github.com/terraform-providers/terraform-provider-google/pull/1385/files#diff-c00013899b923b404c727949952f35ffR172 ?
The reason I didn't do that is because I need the struct accessible so I can generate the ID using it.
subnetwork.Region = region | ||
d.SetId(createSubnetID(subnetwork)) | ||
d.SetId(createSubnetID(subnetworkV1)) |
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.
Should we set the id based on which subnet version (beta or release api) the request was sent to? Or is the id not generated from the api?
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.
The ID is (by design) the same between beta and v1; it's just the project and subnetwork, if I recall correctly. Basically, createSubnetID is a helper function for fmt.Sprintf("%s/%s/%s", subnetworkV1.Project, subnetworkV1.Region, subnetworkV1.Name)
or what have you. So it's entirely generated from user input.
Just noticed I neglected to update the docs for this, I'll push a docs update momentarily. |
Docs are pushed, this is ready if anyone wants to take another look at 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.
Assuming the acceptance tests are passing.
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 👉 [email protected]. Thanks! |
Add the
enable_flow_logs
field to our subnetwork resource, so we canspecify whether flow logs should be enabled in Terraform configs.
Note that this behavior isn't explicitly documented yet, but it has made
it into the beta API client.
Tests: