-
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
New Data Source: aws_workspaces_image #11428
Conversation
Unfortunately, it's not possible to create an image with the API at the moment, I've requested corresponding API action with AWS Support ticket. |
276e7a4
to
d4d041d
Compare
resp, err := conn.DescribeWorkspaceImages(input) | ||
if err != nil { | ||
return err | ||
} |
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.
Will probably want to check len(resp.Images) > 0
here.
See #11837.
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, will add
51ca002
to
dc2f918
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.
Looks good, @Tensho. A couple changes to make.
Providers: testAccProviders, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccDataSourceAwsWorkspacesImageConfig("wsi-vwtykwqyx"), |
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.
This won't pass in our testing environments, since the image is owned by your account.
We could add an environment variable for the image ID and have a pre-check function that checks for the environment variable. Alternatively, there are API functions to import AMIs as Workspace images.
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 like the idea with environment variable, will add the pre-check for now. Unfortunately, ImportWorkspaceImage API works only with Windows. I've submitted respective feature request to AWS Support last year. Is it OK to create aws_workspace_image
Terraform resource only for Windows, while AWS supports Linux images too?
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.
Yes, we can do that. We'll just need to document that it only supports Windows images because of the AWS API
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.
Unfortunately, even Windows images could be imported only with BYOL, which looks like a totally separate feature.
dc2f918
to
6ed0c56
Compare
@gdavison Thank you, I've added fixes according to your review, please let me know if I can do anything else here 🙇 |
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.
The test will need to be updated to handle arbitrary image ids
resource.TestCheckResourceAttr(dataSourceName, "image_id", "wsi-vwtykwqyx"), | ||
resource.TestCheckResourceAttr(dataSourceName, "name", "Windows10Baseline"), | ||
resource.TestCheckResourceAttr(dataSourceName, "description", "Google Chrome AWS WorkDocs Drive CrowdStrike Falcon Russian/Ukrainian Language Pack"), | ||
resource.TestCheckResourceAttr(dataSourceName, "os", "WINDOWS"), | ||
resource.TestCheckResourceAttr(dataSourceName, "required_tenancy", "DEFAULT"), | ||
resource.TestCheckResourceAttr(dataSourceName, "state", "AVAILABLE"), |
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.
Since we're retrieving a WorkSpaces Image using an arbitrary ID, we won't know these values beforehand. You might be able to test this by retrieving the API resource by ID, and then comparing the values in the Terraform resource state. testAccCheckEfsMountTarget()
is an example of the approach you could take
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.
testAccCheckEfsMountTarget()
retrieves actual resource attributes within AWS API, but doesn't test them against state 😄 But I got the idea, you meant item 2 at Anatomy of an Acceptance Test – testAccCheckCloudWatchDashboardExists()
.
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.
Fixed
|
||
func testAccWorkspacesImagePreCheck(t *testing.T) { | ||
if os.Getenv("AWS_WORKSPACES_IMAGE_ID") == "" { | ||
t.Fatal("AWS_WORKSPACES_IMAGE_ID env var must be set for AWS WorkSpaces image acceptance tests. This is required until AWS provides ubiquitous (Windows, Linux) import image API.") |
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.
If the environment variable isn't set, this should skip the test instead of failing
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.
Fixed
4bf7c79
to
bc6e5a5
Compare
@gdavison FIxed, check it out again. |
bc6e5a5
to
5fc6e61
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.
Looking good. It will need to be rebased, and I have a few suggested changes.
"state": { | ||
Type: schema.TypeString, | ||
Computed: true, | ||
}, |
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.
We can remove the state
attribute, since it is more of a run-time property, not a configuration property.
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.
But this is a data source, not a resource. Don't we need to get info about any possible attribute there?
5fc6e61
to
b775925
Compare
b775925
to
b4ba8f2
Compare
d0e321f
to
e4e2c52
Compare
@gdavison @ewbankkit Hi ^_^ Please could you check it again? |
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 🚀
--- PASS: TestAccDataSourceAwsWorkspacesImage_basic (54.32s)
@gdavison Thank you 🙇 |
This has been released in version 3.8.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
Release note for CHANGELOG:
New Data Source:
aws_workspaces_image
(#11428)Example
TODO
aws_workspaces_image
resource before inspecting it with the described data sourceAcceptance Tests
References