Skip to content
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

Merged
merged 4 commits into from
Jan 2, 2020

Conversation

JamesPeiris
Copy link
Contributor

@JamesPeiris JamesPeiris commented Aug 15, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #9782

Release note for CHANGELOG:

NONE

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudWatchDashboard_updateName'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSCloudWatchDashboard_updateName -timeout 120m
=== RUN   TestAccAWSCloudWatchDashboard_updateName
=== PAUSE TestAccAWSCloudWatchDashboard_updateName
=== CONT  TestAccAWSCloudWatchDashboard_updateName
--- PASS: TestAccAWSCloudWatchDashboard_updateName (35.34s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	35.384s
...

FYI:

$ make testacc TESTARGS='-run=TestAccXXX'

Without the TEST=./aws env var also set results in:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSCloudWatchDashboard_updateName -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]

Which hangs until killed.

Thanks in advance!

@JamesPeiris JamesPeiris requested a review from a team August 15, 2019 14:17
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/cloudwatch Issues and PRs that pertain to the cloudwatch service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Aug 15, 2019
@bflad bflad added the bug Addresses a defect in current functionality. label Dec 19, 2019
Copy link
Contributor

@bflad bflad left a 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. 👍

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 19, 2019
@bflad bflad self-assigned this Dec 19, 2019
@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Jan 2, 2020
@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 2, 2020
@JamesPeiris
Copy link
Contributor Author

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 🎉

Copy link
Contributor

@bflad bflad left a 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),
Copy link
Contributor

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:

https://github.com/terraform-providers/terraform-provider-aws/blob/e1fc857056e66b257520f73ca5e22dc6facaebef/aws/resource_aws_cloudwatch_dashboard_test.go#L159

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. 👍

Copy link
Contributor Author

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!

@bflad bflad added this to the v2.44.0 milestone Jan 2, 2020
bflad added a commit that referenced this pull request Jan 2, 2020
…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)
```
@bflad bflad merged commit 5c1a194 into hashicorp:master Jan 2, 2020
bflad added a commit that referenced this pull request Jan 2, 2020
@JamesPeiris JamesPeiris deleted the bugfix/cloudwatch-dashboards branch January 2, 2020 16:30
@ghost
Copy link

ghost commented Jan 10, 2020

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!

@ghost
Copy link

ghost commented Mar 27, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/cloudwatch Issues and PRs that pertain to the cloudwatch service. size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remnant CloudWatch Dashboards persist on each dashboard rename
2 participants