-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix the out of boundary error #9628
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. Terraform GA: Diff ( 2 files changed, 56 insertions(+), 1 deletion(-)) |
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 testsTestAccMonitoringDashboard_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.
small suggestion for release note:
error -> crash ?
Also thanks for adding the test scenario!
The release note says |
@@ -242,6 +251,52 @@ EOF | |||
`) | |||
} | |||
|
|||
func testAccMonitoringDashboard_gridLayoutUpdate() 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.
Does the addition of this test mean that updating the grid layout is sufficient to trigger the index out of range crash?
If so, thank you for adding this to prevent a future regression!
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.
Actually, it appears there is already testAccMonitoringDashboard_gridLayout
- maybe this deserves a comment as to how this is different? Fwiw, I can't tell from the name how the intention behind testAccMonitoringDashboard_gridLayoutUpdate
is supposed to be different from testAccMonitoringDashboard_gridLayout
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.
This test triggers the index out of range crash. With the fix in this PR, this test passed.
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.
Yeah, the comment will help.
ResourceName: "google_monitoring_dashboard.dashboard", | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
ImportStateVerifyIgnore: []string{"project"}, |
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.
What do each of these fields e.g. ImportState
do? I read what appears to be the relevant document at https://googlecloudplatform.github.io/magic-modules/develop/test/ - perhaps I am missing something?
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.
These fields are used to test if importing the resource works or not.
ImportState
triggers the import testing mode. ImportStateVerify
will compare the state after import with the server side response. ImportStateVerifyIgnore
will not compare the ignored fields.
"perma-diff" function here means the DiffSuppressFunc I modified the release note to make it more clear. |
fixes hashicorp/terraform-provider-google#16439
Release Note Template for Downstream PRs (will be copied)