-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Auto generate import id for IAM resource tests #15306
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
Auto generate import id for IAM resource tests #15306
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
7f6d491 to
f0889e2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
*IamGenerated test failures are definitely related: Seems like the resource name parameter is not correct, but I'm not sure where to get the correct one from based on @melinath 's comment |
melinath
left a comment
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.
@trodge I think this is probably pretty close. I'd recommend checking what's happening on import for the failing tests vs the successful equivalent in nightlies. For example, TestAccHealthcareConsentStoreIamPolicyGenerated.
In this PR it's trying to import:
/v1beta1/projects/ci-test-project-188019/locations/us-central1/datasets/tf-test-my-datasetzdt37nbfyb%20projects/ci-test-project-188019/locations/us-central1/datasets/tf-test-my-datasetzdt37nbfyb/consentStores/tf-test-my-consent-storezdt37nbfyb:getIamPolicy
vs in nightlies:
/v1beta1/projects/{project}/locations/us-central1/datasets/tf-test-my-datasetgbc74ngpjk/consentStores/tf-test-my-consent-storegbc74ngpjk
Looks like there's an extra %20 in the first one, plus it's putting in the full path of the dataset and then also the full path of the consent store, when it only actually needs the consent store.
(It's possible you'll need to convert full URLs to shortnames for each part as part of the func you're making.)
mmv1/templates/terraform/examples/base_configs/iam_test_file.go.tmpl
Outdated
Show resolved
Hide resolved
a519602 to
9335680
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
fcf7309 to
655b664
Compare
|
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.
|
This comment was marked as outdated.
This comment was marked as outdated.
Tests analyticsTotal tests: 3094 Click here to see the affected service packages
Action takenFound 82 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
/gcbrun |
|
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: 3095 Click here to see the affected service packages
Action takenFound 78 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
This comment was marked as outdated.
This comment was marked as outdated.
|
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.
|
Co-authored-by: Stephen Lewis (Burrows) <[email protected]>
422ccd3 to
f8e0259
Compare
f8e0259 to
ccb34ab
Compare
|
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.
|
melinath
left a comment
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.
LGTM assuming there are no related test failures. If it were possible for us to eliminate the custom import state id funcs in the future that would be great, but it seems like a reasonable cost for the benefit for now.
Tests analyticsTotal tests: 5851 Click here to see the affected service packages
Action takenFound 35 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. |
5d83324
fixes hashicorp/terraform-provider-google#8712
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.