-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
add numeric_id
field into google_compute_network
resource
#9473
add numeric_id
field into google_compute_network
resource
#9473
Conversation
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAttachedCluster_update|TestAccContainerAttachedCluster_containerAttachedClusterFullExample|TestAccDataprocJobIamPolicy|TestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
numberic_id
field into google_compute_network.network
resourcenumeric_id
field into google_compute_network.network
resource
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. Terraform GA: Diff ( 2 files changed, 52 insertions(+)) |
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. Terraform GA: Diff ( 3 files changed, 122 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
Checks are failing at the moment:
|
In my latest commit i addressed the value checks for |
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. Terraform GA: Diff ( 3 files changed, 122 insertions(+)) |
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. Terraform GA: Diff ( 3 files changed, 123 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeNetwork_numericId|TestAccDataSourceGoogleServiceAccountAccessToken_basic |
Rerun these tests in REPLAYING mode to catch issues
|
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 don't think the current approach is the best way to address the problem, but to be fair adding fields that don't map to the API is confusing and requires a bunch of familiarity with the templating in MMv1! Gotta learn the rules to be able to break them 😉
If you'd like to pair on this issue please let me know, I'm available to help synchronously with some planning!
mmv1/templates/terraform/custom_flatten/compute_network_numeric_id.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/compute/resource_compute_network_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/compute/resource_compute_network_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/compute/resource_compute_network_test.go.erb
Outdated
Show resolved
Hide resolved
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. Terraform GA: Diff ( 3 files changed, 115 insertions(+)) |
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. Terraform GA: Diff ( 3 files changed, 115 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleServiceAccountAccessToken_basic |
Rerun these tests in REPLAYING mode to catch issues
|
mmv1/third_party/terraform/services/compute/resource_compute_network_test.go.erb
Outdated
Show resolved
Hide resolved
numeric_id
field into google_compute_network.network
resourcenumeric_id
field into google_compute_network
resource
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. Terraform GA: Diff ( 3 files changed, 82 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccGKEHub2Fleet_gkehubFleetBasicExample_update |
Rerun these tests in REPLAYING mode to catch issues
|
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.
Approved, and I'll leave it to you to merge! I've left a comment about the regex that you could push a commit for before merging, up to you. Thanks for taking the time to update the tests!
{ | ||
Config: testAccComputeNetwork_basic(suffixName), | ||
Check: resource.ComposeTestCheckFunc( | ||
resource.TestMatchResourceAttr("google_compute_network.bar", "numeric_id",regexp.MustCompile("[1-9]\\d{0,}")), |
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: this regex could be made a bit more strict by updating it to assert that only digits are in the value: ^[1-9]\d{0,}$
. Currently the regex would match foobar123
, which means it could say that projects/foobar/global/networks/foobar123
is a valid numeric_id value.
It could also be simplified down to ^\d{1,}$
which means 1 or more digits, whereas the other version looks for a digit and then 0 or more digits afterwards.
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.
My favourite website for doing quick manual tests of regexes is https://regexr.com/
Though, just to be clear, if you're testing a regex that uses ^ and $ the regex will need to match everything in the text box:
![](https://private-user-images.githubusercontent.com/15078782/288747225-975ddbae-641e-4edd-9f54-6c67142bfb38.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxNDcwNzEsIm5iZiI6MTcyNjE0Njc3MSwicGF0aCI6Ii8xNTA3ODc4Mi8yODg3NDcyMjUtOTc1ZGRiYWUtNjQxZS00ZWRkLTlmNTQtNmM2NzE0MmJmYjM4LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEyVDEzMTI1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTAwNWQ2NjBmZThkODcxMGY0YWMyMDY0YjA2ZDE2YzcyMzk1NzQ4ZTBmZjhmYjRlNDhlYTRkYjg1ZTIyNGFkNWQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.I2aSvXFjah_VOyzPg7Dyxm8udGIV2ubIJ15mCtbp2ZQ)
![](https://private-user-images.githubusercontent.com/15078782/288747234-21337a21-50b3-4434-9696-fb4b6b47bb20.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYxNDcwNzEsIm5iZiI6MTcyNjE0Njc3MSwicGF0aCI6Ii8xNTA3ODc4Mi8yODg3NDcyMzQtMjEzMzdhMjEtNTBiMy00NDM0LTk2OTYtZmI0YjZiNDdiYjIwLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEyVDEzMTI1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWFjOTM0NzJmNTRkZmQ4NDNiNTk4ZmJjYjAzN2VjNmZjODFhMWQyNDc1MjQ4ZmFjN2FmNGJhOWY1YjEyYTA2N2ImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.pbl_-_9F2qJHkUfCl4MV6FrLDOuCcXkzTYwNLE3nzvM)
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. Terraform GA: Diff ( 3 files changed, 82 insertions(+)) |
Tests analyticsTotal tests:
|
…CloudPlatform#9473) * add numberic_id field * add flattener, encoders, and id field for numeric_id use * WIP: add numberId test * add value checks for id and numeric_id * add strconv * use decoder to store id into numericId after API request * remove network check and add suffix variable * add endlines * typo * Updated test to explicitly check numeric_id and id with ResourceAttr * update regex to be more strict on numeric_id check
…CloudPlatform#9473) * add numberic_id field * add flattener, encoders, and id field for numeric_id use * WIP: add numberId test * add value checks for id and numeric_id * add strconv * use decoder to store id into numericId after API request * remove network check and add suffix variable * add endlines * typo * Updated test to explicitly check numeric_id and id with ResourceAttr * update regex to be more strict on numeric_id check
This PR will fix an issue that relates to tag binding. Where it is only possible to use an id of a resource for binding rather than
name
. Because of the use ofid
within the terraform providers,numeric_id
is used as a field which is the unique resource identifier for the resource.marked as
id
in API documentation but will be named asnumeric_id
in providerRelease Note Template for Downstream PRs (will be copied)