fix resourceDashboardUpdate #34227

dsbibby · 2023-11-02T16:30:07Z

Description

This PR addresses a bug in the aws_quicksight_dashboard resource.

When terraform updates a dashboard resource, two AWS API calls are made. The first updates the dashboard which results in a new dashboard version. The second call should update the published version of the dashboard to the new version.

The bug manifests when the first call takes a little time, which results in the second call returning an error about the dashboard creation still being in progress.

This is because the provider waits for the dashboard status to change to creation successful, however the API call used is checking the current published version, not the new version just made. As such it returns early and doesn’t actually wait.

This PR fixes this by explicitly waiting for the new version of the dashboard to be created before trying to update the published version.

Additionally this PR fixes two minor typo issues, and an issue caused when the state file for a template contains an empty string, resulting in "Error: a number is required" errors.

Relations

Closes #33463
Closes #35109

References

Redacted screenshot showing the failure:

Output from Acceptance Testing

% make testacc TESTS=TestAccQuickSightDashboard PKG=quicksight
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/quicksight/... -v -count 1 -parallel 20 -run='TestAccQuickSightDashboard'  -timeout 360m
?       github.com/hashicorp/terraform-provider-aws/internal/service/quicksight/schema  [no test files]
=== RUN   TestAccQuickSightDashboard_basic
=== PAUSE TestAccQuickSightDashboard_basic
=== RUN   TestAccQuickSightDashboard_disappears
=== PAUSE TestAccQuickSightDashboard_disappears
=== RUN   TestAccQuickSightDashboard_sourceEntity
=== PAUSE TestAccQuickSightDashboard_sourceEntity
=== RUN   TestAccQuickSightDashboard_updateVersionNumber
=== PAUSE TestAccQuickSightDashboard_updateVersionNumber
=== RUN   TestAccQuickSightDashboard_dashboardSpecificConfig
=== PAUSE TestAccQuickSightDashboard_dashboardSpecificConfig
=== CONT  TestAccQuickSightDashboard_basic
=== CONT  TestAccQuickSightDashboard_updateVersionNumber
=== CONT  TestAccQuickSightDashboard_sourceEntity
=== CONT  TestAccQuickSightDashboard_dashboardSpecificConfig
=== CONT  TestAccQuickSightDashboard_disappears
--- PASS: TestAccQuickSightDashboard_disappears (40.98s)
--- PASS: TestAccQuickSightDashboard_dashboardSpecificConfig (45.02s)
--- PASS: TestAccQuickSightDashboard_basic (45.43s)
--- PASS: TestAccQuickSightDashboard_sourceEntity (48.19s)
--- PASS: TestAccQuickSightDashboard_updateVersionNumber (95.68s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/quicksight 99.768s

github-actions · 2023-11-02T16:30:21Z

Community Note

Voting for Prioritization

Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
Please see our prioritization guide for information on how we prioritize.
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

github-actions

Welcome @dsbibby 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

dsbibby · 2024-01-18T07:00:53Z

Hi, how do I encourage a review of this PR please? It's a pretty minor change to address a nasty bug that is stopping us (and surely others) from using this.

dev-slatto · 2024-03-01T14:45:10Z

internal/service/quicksight/dashboard.go

@@ -203,7 +203,7 @@ func resourceDashboardRead(ctx context.Context, d *schema.ResourceData, meta int
 		return diag.FromErr(err)
 	}

-	out, err := FindDashboardByID(ctx, conn, d.Id())
+	out, err := FindDashboardByID(ctx, conn, d.Id(), -1)


Do we always want the latest version as this is hardcoded?

The issue this MR tries to fix is urgent since the current provider version makes it impossible to update a dashboard.

To update a dashboard we currently use the workaround to rename the dashboard, hence deleting the old and creating a new dashboard. This has the impact of making the user data, such as bookmarks, customisation, etc, vanish for the user and therefore not making it very user-friendly.

@dev-slatto I struggle to understand the concern you are raising. The old version of the code also tries to update to the latest version and it does not even allow you to update the dashboard at all since it now throws the error referred to in this MR.

When deploying a new version it would be (in my opinion) natural to deploy the latest version in most of the use cases. I struggle to see a good use case for not enabling the latest version except for maybe testing the new version before it is published to everyone. We have separate dev/test/prod env, so for us, it makes sense to always publish the latest.

Do we always want the latest version as this is hardcoded?

That is the current (as in before this PR) behaviour, so no change to the current behaviour.

That said, we could easily add a version parameter to this resourceDashboardRead() function if that would be beneficial for the future?

Had a chat with Graham and we concluded that it makes sence for a hardcoded version for the resource, and that a data source of dashboard should be able to include the version parameter. However, see the comment later down in the PR about defining the version as a constant instead of hardcoding it 😄

Great, I have updated to use a constant 👍

gdavison

Thanks for the PR, @dsbibby. It looks good, though I have a few suggestions

gdavison · 2024-03-08T18:37:43Z

internal/service/quicksight/dashboard.go

@@ -367,15 +368,26 @@ func resourceDashboardDelete(ctx context.Context, d *schema.ResourceData, meta i
 	return nil
 }

-func FindDashboardByID(ctx context.Context, conn *quicksight.QuickSight, id string) (*quicksight.Dashboard, error) {
+// Pass version as -1 for latest published version, or specify a specific version if required


We should define a constant, e.g. dashboardLatestVersion instead of using -1

gdavison · 2024-03-08T18:42:19Z

internal/service/quicksight/dashboard.go

+	var descOpts *quicksight.DescribeDashboardInput
+
+	if version == -1 {
+		descOpts = &quicksight.DescribeDashboardInput{
+			AwsAccountId: aws.String(awsAccountId),
+			DashboardId:  aws.String(dashboardId),
+		}
+	} else {
+		descOpts = &quicksight.DescribeDashboardInput{
+			AwsAccountId:  aws.String(awsAccountId),
+			DashboardId:   aws.String(dashboardId),
+			VersionNumber: &version,
+		}
 	}


It might be more clear to understand to have something like

Suggested change

var descOpts *quicksight.DescribeDashboardInput

if version == -1 {

descOpts = &quicksight.DescribeDashboardInput{

AwsAccountId: aws.String(awsAccountId),

DashboardId: aws.String(dashboardId),

}

} else {

descOpts = &quicksight.DescribeDashboardInput{

AwsAccountId: aws.String(awsAccountId),

DashboardId: aws.String(dashboardId),

VersionNumber: &version,

}

}

descOpts := &quicksight.DescribeDashboardInput{

AwsAccountId: aws.String(awsAccountId),

DashboardId: aws.String(dashboardId),

}

if version != dashboardLatestVersion {

descOpts.VersionNumber = aws.Int64(version)

}

Yes, that's neater, done 👍

gdavison · 2024-03-08T18:48:45Z

internal/service/quicksight/dashboard.go

@@ -297,14 +297,15 @@ func resourceDashboardUpdate(ctx context.Context, d *schema.ResourceData, meta i
 			return create.DiagError(names.QuickSight, create.ErrActionUpdating, ResNameDashboard, d.Id(), err)
 		}

-		if _, err := waitDashboardUpdated(ctx, conn, d.Id(), d.Timeout(schema.TimeoutUpdate)); err != nil {
+		updatedVersionNumber := extractVersionFromARN(aws.StringValue(out.VersionArn))


Because of how extractVersionFromARN is used here, it should return int64 instead of *int64. That way, the parameter will not need to be dereferenced when passed to waitDashboardUpdated. When setting the parameter on publishVersion, use aws.Int64(updatedVersionNumber)

Have addressed this 👍

gdavison · 2024-03-08T20:02:29Z

internal/service/quicksight/dashboard_test.go

+					testAccCheckDashboardVersionExists(ctx, resourceName, 1, &dashboard),
+					testAccCheckDashboardName(&dashboard, rName),


For clarity, these should indicate that they're referencing version 1, e.g.

Suggested change

testAccCheckDashboardVersionExists(ctx, resourceName, 1, &dashboard),

testAccCheckDashboardName(&dashboard, rName),

testAccCheckDashboardVersionExists(ctx, resourceName, 1, &dashboardV1),

testAccCheckDashboardName(&dashboardV1, rName),

Updated to dashboardV1

dsbibby · 2024-03-12T22:55:23Z

I have addressed the comments so far. In testing this with a massive config I have access to, I got blocked by a couple of minor typos for which there are existing issues and a regression introduced by #33931 - I added fixes for these also so that I could complete my tests. Hope that is OK.

gdavison

Looks good! 🚀

--- PASS: TestAccQuickSightDashboard_disappears (31.92s)
--- PASS: TestAccQuickSightDashboard_dashboardSpecificConfig (32.84s)
--- PASS: TestAccQuickSightDashboard_sourceEntity (35.41s)
--- PASS: TestAccQuickSightDashboard_basic (35.93s)
--- PASS: TestAccQuickSightDashboard_updateVersionNumber (57.83s)

github-actions · 2024-03-28T21:50:48Z

This functionality has been released in v5.43.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. Thank you!

github-actions · 2024-04-28T02:10:10Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/quicksight Issues and PRs that pertain to the quicksight service. size/S Managed by automation to categorize the size of a PR. labels Nov 2, 2023

terraform-aws-provider bot added the needs-triage Waiting for first response or review from a maintainer. label Nov 2, 2023

github-actions bot reviewed Nov 2, 2023

View reviewed changes

dsbibby marked this pull request as ready for review November 3, 2023 15:09

dsbibby force-pushed the b-aws_quicksight_dashboard-fix-update-bug branch from 749b261 to 4ae51ba Compare November 3, 2023 15:41

justinretzolk added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Nov 7, 2023

dev-slatto reviewed Mar 1, 2024

View reviewed changes

gdavison self-assigned this Mar 8, 2024

terraform-aws-provider bot added the prioritized Part of the maintainer teams immediate focus. To be addressed within the current quarter. label Mar 8, 2024

gdavison requested changes Mar 8, 2024

View reviewed changes

github-actions bot added size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Mar 12, 2024

dsbibby added 7 commits March 12, 2024 22:57

fix resourceDashboardUpdate

6b5c953

add changelog

0b43add

update unit tests

3028f4a

refactor: address PR comments

36f8bcd

refactor: address PR comments

d552ebd

fix: typos

55b54be

chore: update changelog

061a321

dsbibby force-pushed the b-aws_quicksight_dashboard-fix-update-bug branch from 2d8cacc to 061a321 Compare March 12, 2024 22:58

chore: linting fixes

6cf30f5

gdavison approved these changes Mar 22, 2024

View reviewed changes

gdavison merged commit 74bad20 into hashicorp:main Mar 22, 2024
36 checks passed

github-actions bot added this to the v5.43.0 milestone Mar 22, 2024

github-actions bot removed the prioritized Part of the maintainer teams immediate focus. To be addressed within the current quarter. label Mar 28, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix resourceDashboardUpdate #34227

fix resourceDashboardUpdate #34227

dsbibby commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023

github-actions bot left a comment

dsbibby commented Jan 18, 2024

dev-slatto Mar 1, 2024

atlesp Mar 5, 2024

dsbibby Mar 5, 2024

dev-slatto Mar 9, 2024 •

edited

Loading

dsbibby Mar 12, 2024

gdavison left a comment

gdavison Mar 8, 2024

dsbibby Mar 12, 2024

gdavison Mar 8, 2024

dsbibby Mar 12, 2024

gdavison Mar 8, 2024

dsbibby Mar 13, 2024

gdavison Mar 8, 2024

dsbibby Mar 12, 2024

dsbibby commented Mar 12, 2024

gdavison left a comment

github-actions bot commented Mar 28, 2024

github-actions bot commented Apr 28, 2024

		testAccCheckDashboardVersionExists(ctx, resourceName, 1, &dashboard),
		testAccCheckDashboardName(&dashboard, rName),

fix resourceDashboardUpdate #34227

fix resourceDashboardUpdate #34227

Conversation

dsbibby commented Nov 2, 2023 • edited Loading

Description

Relations

References

Output from Acceptance Testing

github-actions bot commented Nov 2, 2023

Community Note

github-actions bot left a comment

Choose a reason for hiding this comment

dsbibby commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dev-slatto Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsbibby commented Mar 12, 2024

gdavison left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2024

github-actions bot commented Apr 28, 2024

dsbibby commented Nov 2, 2023 •

edited

Loading

dev-slatto Mar 9, 2024 •

edited

Loading