-
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
[aws_workspaces_workspace] Add Failed Request Error Code Along With Message #16459
Conversation
00ae0cb
to
5fb754d
Compare
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.
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)
@@ -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) |
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.
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.
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.
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
.
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.
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.
5fb754d
to
5e22dfc
Compare
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.
LGTM (codewise)
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 |
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! |
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 #15317
Release note for CHANGELOG:
Acceptance Tests
TBH, I don't know how to simulate workspace creation/termination failed requests in a predictable way ¯\(ツ)/¯.