tpgresource: added support for default location in tpgresource.ReplaceVars
#12320
Conversation
…ceVars`
This supports inferring `{{location}}` from the provider (similar to
what is done with `{{region}}` and `{{zone}}` in
`tpgresource.ReplaceVars()`
Part of hashicorp/terraform-provider-google#19266
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @zli82016, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
modular-magician
added
the
awaiting-approval
modular-magician
removed
the
awaiting-approval
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
|
I'll take a look at the TGC unit test failures later as well. But would be good to get some input from whoever has some institutional knowledge about this area of the codebase. |
Tests analyticsTotal tests: 4292 Click here to see the affected service packages
Action takenFound 51 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
It becomes much more complicated then the original issue and some other tests failed. It looks like there are some special cases for the location field in some resources. Do you want to just fix the import issue for |
|
Unless it's something very trivially fixable, I'm probably going to move this one to draft soon., but if you have ideas (or if there are folks you can pull in who have ideas) about generally speaking whether this is a good idea, and how it should work), would appreciate the comments. I did take a look at some of the tests, and at least the cloudbuild ones seem to fail for me off of |
|
@zli82016 I'm definitely going to punt on this for now. I'm not sure if there are any easy fixes for the |
That is the reason for the tests failure in Also, I checked the resource |
This supports inferring
{{location}}from the provider (similar to what is done with{{region}}and{{zone}}intpgresource.ReplaceVars(), vs. just taking a value if it's explicitly set.Even if all tests pass, it's probably necessary for someone who understands how this is used to weigh in and make sure this isn't likely to have any unintended consequences / side effects.
While
ReplaceVars()is used all over the place, like the replacements for{{region}}and{{zone}},BuildReplacementFunc()is using a method (GetLocation(), in this case) that was seemingly originally intended for use with GKE clusters, but probably generically will work in some situations.In this case, for an artifact registry repository resource with a location set at provider level, this fixes some issues with import if the resource is in the same region that's configured at provider level.
Part of hashicorp/terraform-provider-google#19266
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.