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

[aws_workspaces_workspace] Add Failed Request Error Code Along With Message #16459

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

Tensho
Copy link
Contributor

@Tensho Tensho commented Nov 27, 2020

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #15317

Release note for CHANGELOG:

aws_workspaces_workspace: Add failed request error code along with message

Acceptance Tests

TBH, I don't know how to simulate workspace creation/termination failed requests in a predictable way ¯\(ツ)/¯.

AWS_REGION=us-east-1 make testacc TEST=./aws TESTARGS='-run=TestAccAwsWorkspacesWorkspace_basic'       okta:sandbox
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAwsWorkspacesWorkspace_basic -timeout 120m
=== RUN   TestAccAwsWorkspacesWorkspace_basic
=== PAUSE TestAccAwsWorkspacesWorkspace_basic
=== CONT  TestAccAwsWorkspacesWorkspace_basic
--- PASS: TestAccAwsWorkspacesWorkspace_basic (2082.75s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2084.673s

@Tensho Tensho requested a review from a team as a code owner November 27, 2020 08:02
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/workspaces Issues and PRs that pertain to the workspaces service. labels Nov 27, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Nov 27, 2020
@anGie44 anGie44 added the technical-debt Addresses areas of the codebase that need refactoring or redesign. label Dec 3, 2020
Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

A couple suggestions.

--- PASS: TestFlattenWorkspaceProperties (0.00s)
--- PASS: TestExpandWorkspaceProperties (0.00s)
--- PASS: TestAccAwsWorkspacesWorkspace_validateUserVolumeSize (3.46s)
--- PASS: TestAccAwsWorkspacesWorkspace_validateRootVolumeSize (3.87s)
--- PASS: TestAccAwsWorkspacesWorkspace_timeout (1766.42s)
--- PASS: TestAccAwsWorkspacesWorkspace_tags (1785.27s)
--- PASS: TestAccAwsWorkspacesWorkspace_recreate (1800.49s)
--- PASS: TestAccAwsWorkspacesWorkspace_basic (1812.50s)
--- PASS: TestAccAwsWorkspacesWorkspace_workspaceProperties (1832.72s)
--- PASS: TestAccAwsWorkspacesWorkspace_workspaceProperties_runningModeAlwaysOn (1855.85s)

aws/resource_aws_workspaces_workspace.go Outdated Show resolved Hide resolved
@@ -171,7 +171,7 @@ func resourceAwsWorkspacesWorkspaceCreate(d *schema.ResourceData, meta interface

wsFail := resp.FailedRequests
if len(wsFail) > 0 {
return fmt.Errorf("workspace creation failed: %s", *wsFail[0].ErrorMessage)
return fmt.Errorf("workspace creation failed: %s: %s", *wsFail[0].ErrorCode, *wsFail[0].ErrorMessage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed, but it could be useful to create a function that consistently formats the message. It's unfortunate that the values are in struct fields and not function calls, meaning that we can't use an interface.

But something like

func failedCreateWorkspaceRequestString(wsFail *workspaces.FailedCreateWorkspaceRequest) {
    return "%s: %s", aws.StringValue(wsFail.ErrorCode), aws.StringValue(wsFail.ErrorMessage)
}

func failedWorkspaceChangeRequestString(wsFail *workspaces.FailedWorkspaceChangeRequest) {
    return "%s: %s", aws.StringValue(wsFail.ErrorCode), aws.StringValue(wsFail.ErrorMessage)
}

and then

return fmt.Errorf("workspace creation failed: %s", failedCreateWorkspaceRequestString(wsFail[0]))

could be used to ensure that we're consistent everywhere.

Copy link
Contributor Author

@Tensho Tensho Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point 👍 I guess you mean it would be nice to have something like this for the failed requests. AWS WorkSpaces service provides FailedCreateWorkspaceRequest and FailedWorkspaceChangeRequest data types only for workspace entity. Right now both of them are handled in one file, so I'd prefer to create the proposed abstraction once we get more FailedXXXRequest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally 👍 to also not creating these abstractions unless there is multiple usage -- there are a lot of service APIs that have similar "error code and message" fields within regular structure shapes since they are different than the shapes for modeled errors, so it could be a lot of abstracting for minimal benefit.

@gdavison gdavison added this to the v3.20.0 milestone Dec 3, 2020
@gdavison gdavison removed the needs-triage Waiting for first response or review from a maintainer. label Dec 3, 2020
@gdavison gdavison self-assigned this Dec 3, 2020
@breathingdust breathingdust modified the milestones: v3.20.0, v3.21.0 Dec 3, 2020
Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (codewise)

@breathingdust
Copy link
Member

LGTM 🚀 Thanks @Tensho

Verified Acceptance Tests in Commercial (us-west-2)

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

@breathingdust breathingdust merged commit c5a8e71 into hashicorp:master Dec 8, 2020
breathingdust added a commit that referenced this pull request Dec 8, 2020
@Tensho Tensho deleted the workspaces-error-codes branch December 8, 2020 17:26
@ghost
Copy link

ghost commented Dec 11, 2020

This has been released in version 3.21.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 Jan 8, 2021

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 as resolved and limited conversation to collaborators Jan 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/workspaces Issues and PRs that pertain to the workspaces service. size/XS Managed by automation to categorize the size of a PR. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workspaces error handling should include ErrorCode, not just ErrorMessage
6 participants