-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fixed remnant CloudWatch Dashboards on each dashboard rename #9784
Fixed remnant CloudWatch Dashboards on each dashboard rename #9784
Conversation
…ded related test.
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.
Hi @JamesPeiris 👋 Thank you for submitting this fix.
Usually in these situations it is easiest to just add ForceNew: true
to the problematic schema attribute (dashboard_name
in this case), which should hopefully eliminate most, if not all the other changes since Terraform will automatically plan on destroying the old dashboard and creating a new dashboard.
Can you please try updating this pull request with that instead? Thanks. 👍
Hello @bflad 👋 Thank you for taking the time to look at this, I appreciate that your time must be spread quite thinly! I have amended the implementation as per your suggestion, which I have tested and works as expected - would you be able to take another look at this please? Thanks again, and happy new year 🎉 |
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.
Thanks so much for the update, @JamesPeiris 🚀 One minor testing fix on merge, which I will take care of for you.
Output from acceptance testing after testing fix:
--- PASS: TestAccAWSCloudWatchDashboard_basic (19.86s)
--- PASS: TestAccAWSCloudWatchDashboard_updateName (31.01s)
--- PASS: TestAccAWSCloudWatchDashboard_update (33.48s)
{ | ||
Config: testAccAWSCloudWatchDashboardConfig(rInt), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckCloudWatchDashboardExists("aws_cloudwatch_dashboard.foobar", &dashboard), |
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.
Likely copy-paste from the earlier iteration of this pull request, the test configuration resource name is aws_cloudwatch_dashboard.test
:
Otherwise, this test causes this error:
--- FAIL: TestAccAWSCloudWatchDashboard_updateName (5.64s)
testing.go:640: Step 0 error: Check failed: Check 1/3 error: Not found: aws_cloudwatch_dashboard.foobar
Since it is a small testing fix, will do so on merge. 👍
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.
Gah, I was so close. Thanks!
…ashboard_updateName resource name to match configuration Reference: #9784 Output from acceptance testing: ``` --- PASS: TestAccAWSCloudWatchDashboard_basic (19.86s) --- PASS: TestAccAWSCloudWatchDashboard_updateName (31.01s) --- PASS: TestAccAWSCloudWatchDashboard_update (33.48s) ```
This has been released in version 2.44.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Closes #9782
Release note for CHANGELOG:
Output from acceptance testing:
FYI:
Without the
TEST=./aws
env var also set results in:Which hangs until killed.
Thanks in advance!