-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Handle aggregatedList case for generated sweepers #2941
Handle aggregatedList case for generated sweepers #2941
Conversation
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. |
e47130c
to
d290886
Compare
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. |
362e02c
to
a21ec40
Compare
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. |
@@ -13,6 +13,12 @@ import ( | |||
<% | |||
sweeper_name = product_ns + object.name | |||
wrap_path = object&.nested_query&.keys&.first || object.collection_url_key | |||
listUrlTemplate = object.__product.base_url + object.base_url | |||
|
|||
listUrlTemplate.sub! "zones/{{zone}}", "aggregated" |
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'm assuming there's basically no non-GCE zonal resources, so we'll only replace GCE resources with 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.
Yep, other resources use location/
in the base url and they don't support aggregatedList. I am assuming some consistency in the future, otherwise we will have to use some manual flag.
log.Printf("[INFO] %s resource zone was nil", resourceName) | ||
return nil | ||
} | ||
zoneSegs := strings.Split(obj["zone"].(string),"/") |
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 think this is just https://github.com/terraform-providers/terraform-provider-google/blob/master/google/self_link_helpers.go#L78, right? Can we use that instead?
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.
Didn't see that helper! I actually do the same thing here to parse the name. I will replace both instances at the same time in a separate PR since it will cause diffs for a ton of existing sweeper files
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
a21ec40
to
5d708a0
Compare
fixes hashicorp/terraform-provider-google#5300
This PR includes:
skip_sweeper
flag from resources that use aggregatedList (generates 5 new sweepers)Edit:
Ran this PR in our CI, you can find the new sweepers running here.
Release Note Template for Downstream PRs (will be copied)