Conversation
|
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.
|
Tests analyticsTotal tests: 4901 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
mmv1/third_party/terraform/provider/universe/universe_domain_compute_test.go
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.
|
Tests analyticsTotal tests: 4917 Click here to see the affected service packages
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 tests
|
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
mmv1/third_party/terraform/provider/universe/universe_domain_compute_test.go
Outdated
Show resolved
Hide resolved
Looks like this test actually passed in https://storage.mtls.cloud.google.com/ci-vcr-logs/beta/refs/heads/auto-pr-13863/artifacts/7b3e92a9-bb66-47fb-a694-79ad3ff89ed0/build-log/replaying_test.log. For some reason it didn't show up on the UI |
| return nil, diag.FromErr(err) | ||
| } | ||
|
|
||
| // Bypass checkDestroyer failure with universe domain set |
There was a problem hiding this comment.
@c2thorn FYI, this is added to bypass check destroy error
There was a problem hiding this comment.
Why does this work exactly? I don't think we're generally expected to call d.Set inside the provider as it's not a full resource, so I'm a bit surprised it even works.
There was a problem hiding this comment.
If this works, this is the exact same as removing the check later in https://github.com/hashicorp/terraform-provider-google/blob/main/google/provider/provider.go#L1266. There's no point in having both.
I was hoping to either set the universe domain in a test-only setup function, or skip the check altogether by adding something to
else if config.UniverseDomain != "" && config.UniverseDomain != "googleapis.com"
that verifies if it is a test run.
There was a problem hiding this comment.
What is the issue we're seeing exactly? Does https://github.com/hao-nan-li/magic-modules/blob/2b5b82e44b188362545a5ca43a854c7397079d75/mmv1/third_party/terraform/provider/universe/universe_domain_compute_test.go#L145 not have the domain set?
There was a problem hiding this comment.
Here's the flow:
- https://github.com/hashicorp/terraform-provider-google/blob/main/google/provider/universe/universe_domain_compute_test.go#L100
- https://github.com/hashicorp/terraform-provider-google/blob/main/google/acctest/provider_test_utils.go#L45
- The configure function then calls https://github.com/hashicorp/terraform-provider-google/blob/ff829fe23a4235e6566fd07be74c52e32e376b69/google/provider/provider.go#L921
Finally an error occurs because universe_domain is not set in d (schema.ResourceData). And the code right below will return an error, then the check destroyer function fails.
Adding this block of code works around this problem
There was a problem hiding this comment.
Discussed offline; we can likely set the value on https://github.com/hashicorp/terraform-provider-google/blob/8ef7e5fbcf4f8d5a2a141dfff29bffc52d119e65/google/acctest/provider_test_utils.go#L44 to later successfully call configure.
More broadly, we should figure out how the two initialization paths (through the provider factory and there) are different, and ideally align them.
There was a problem hiding this comment.
Applied fix in provider_test_utils. And this solves the error in check destroy.
Will take a look at the second suggestion.
|
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.
|
|
the unit test failure looks unrelated to this change, perhaps it's something needed to be fixed... |
|
FYI @ScottSuarez as this may be related to #13929 In the meantime can you rebase this PR on top of |
|
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.
|
|
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.
|
|
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.
|
|
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.
|
Tests analyticsTotal tests: 4924 Click here to see the affected service packages
Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Tests analyticsTotal tests: 4924 Click here to see the affected service packages
Action takenFound 5 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. |
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
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.
|
|
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.
|
Tests analyticsTotal tests: 4934 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Tests analyticsTotal tests: 4938 Click here to see the affected service packages
Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Several tests terminated during RECORDING mode. 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Several tests terminated during RECORDING mode. 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
| } | ||
| } | ||
| if c.UniverseDomain != "" && c.UniverseDomain != "googleapis.com" { | ||
| resolveImageLink = regexp.MustCompile(fmt.Sprintf("^https://compute.%s/compute/[a-z0-9]+/projects/(%s)/global/images/(%s)", c.UniverseDomain, verify.ProjectRegex, resolveImageImageRegex)) |
There was a problem hiding this comment.
This fix works but we should probably make it more clear. Out of band, could we move resolveImageLink into a function that returns the appropriate link for the current universe? That'll guard against direct access of the raw value, move the logic together, and give a good spot to document the differences with a function comment.
| } | ||
|
|
||
| // Project Prefix of different universes | ||
| func GetProjectPrefixFromEnv() string { |
There was a problem hiding this comment.
In a followup PR could you change the env var to include "Universe" in the namespace? i.e. GOOGLE_UNIVERSE_PROJECT_PREFIX / GetUniverseProjectPrefixFromEnv
Co-authored-by: Riley Karson <rileykarson@google.com>
|
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.
|
Tests analyticsTotal tests: 4950 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
1c17945
Co-authored-by: Riley Karson <rileykarson@google.com>
Co-authored-by: Riley Karson <rileykarson@google.com>
Co-authored-by: Riley Karson <rileykarson@google.com>
Co-authored-by: Riley Karson <rileykarson@google.com>
Co-authored-by: Riley Karson <rileykarson@google.com>
Co-authored-by: Riley Karson <rileykarson@google.com>
Fixes hashicorp/terraform-provider-google#22566
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.