New Resource: aws_imagebuilder_image #16710

bflad · 2020-12-11T05:43:54Z

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

Release note for CHANGELOG:

* **New Data Source:** `aws_imagebuilder_image`
* **New Resource:** `aws_imagebuilder_image`

Output from acceptance testing in AWS Commercial:

--- PASS: TestAccAwsImageBuilderImage_basic (1596.87s)
--- PASS: TestAccAwsImageBuilderImage_disappears (1593.54s)
--- PASS: TestAccAwsImageBuilderImage_DistributionConfigurationArn (1844.49s)
--- PASS: TestAccAwsImageBuilderImage_EnhancedImageMetadataEnabled (2636.68s)
--- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_ImageTestsEnabled (2616.36s)
--- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_TimeoutMinutes (3026.17s)
--- PASS: TestAccAwsImageBuilderImage_Tags (1514.62s)

--- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Aws (12.69s)
--- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Self (1458.04s)

Output from acceptance testing in AWS GovCloud (US):

--- PASS: TestAccAwsImageBuilderImage_basic (1463.62s)
--- PASS: TestAccAwsImageBuilderImage_disappears (1460.27s)
--- PASS: TestAccAwsImageBuilderImage_DistributionConfigurationArn (1850.90s)
--- PASS: TestAccAwsImageBuilderImage_EnhancedImageMetadataEnabled (2648.38s)
--- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_ImageTestsEnabled (2648.21s)
--- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_TimeoutMinutes (2914.17s)
--- PASS: TestAccAwsImageBuilderImage_Tags (1643.87s)

--- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Aws (19.19s)
--- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Self (1462.29s)

Reference: #16375 Reference: #16377 Output from acceptance testing in AWS Commercial: ``` --- PASS: TestAccAwsImageBuilderImage_basic (1596.87s) --- PASS: TestAccAwsImageBuilderImage_disappears (1593.54s) --- PASS: TestAccAwsImageBuilderImage_DistributionConfigurationArn (1844.49s) --- PASS: TestAccAwsImageBuilderImage_EnhancedImageMetadataEnabled (2636.68s) --- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_ImageTestsEnabled (2616.36s) --- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_TimeoutMinutes (3026.17s) --- PASS: TestAccAwsImageBuilderImage_Tags (1514.62s) --- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Aws (12.69s) --- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Self (1458.04s) ``` Output from acceptance testing in AWS GovCloud (US): ``` --- PASS: TestAccAwsImageBuilderImage_basic (1463.62s) --- PASS: TestAccAwsImageBuilderImage_disappears (1460.27s) --- PASS: TestAccAwsImageBuilderImage_DistributionConfigurationArn (1850.90s) --- PASS: TestAccAwsImageBuilderImage_EnhancedImageMetadataEnabled (2648.38s) --- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_ImageTestsEnabled (2648.21s) --- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_TimeoutMinutes (2914.17s) --- PASS: TestAccAwsImageBuilderImage_Tags (1643.87s) --- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Aws (19.19s) --- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Self (1462.29s) ```

YakDriver

Few optional nits but otherwise LGTM! 🎉

GovCloud:

--- PASS: TestAccAwsImageBuilderImage_basic (1330.88s)
--- PASS: TestAccAwsImageBuilderImage_disappears (1455.57s)
--- PASS: TestAccAwsImageBuilderImage_DistributionConfigurationArn (1586.72s)
--- PASS: TestAccAwsImageBuilderImage_EnhancedImageMetadataEnabled (1201.09s)
--- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_ImageTestsEnabled (946.34s)
--- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_TimeoutMinutes (1329.30s)
--- PASS: TestAccAwsImageBuilderImage_Tags (1509.61s)
--- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Aws (15.71s)
--- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Self (1454.09s)

us-west-2:

--- PASS: TestAccAwsImageBuilderImage_basic (1332.55s)
--- PASS: TestAccAwsImageBuilderImage_disappears (1326.32s)
--- PASS: TestAccAwsImageBuilderImage_DistributionConfigurationArn (1708.79s)
--- PASS: TestAccAwsImageBuilderImage_EnhancedImageMetadataEnabled (1201.35s)
--- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_ImageTestsEnabled (1075.90s)
--- PASS: TestAccAwsImageBuilderImage_ImageTestsConfiguration_TimeoutMinutes (1328.98s)
--- PASS: TestAccAwsImageBuilderImage_Tags (1246.17s)
--- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Aws (12.57s)
--- PASS: TestAccAwsImageBuilderImageDataSource_Arn_Self (1459.51s)

YakDriver · 2021-01-26T19:37:01Z

aws/data_source_aws_imagebuilder_image.go

+ // To prevent Terraform errors, only reset arn if not configured.
+ // The configured ARN may contain x.x.x wildcards while the API returns
+ // the full build version #.#.#/# suffix.


YakDriver · 2021-01-26T19:41:47Z

aws/data_source_aws_imagebuilder_image_test.go

+ "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+)
+
+func TestAccAwsImageBuilderImageDataSource_Arn_Aws(t *testing.T) {


Suggested change

func TestAccAwsImageBuilderImageDataSource_Arn_Aws(t *testing.T) {

func TestAccAwsImageBuilderImageDataSource_basic(t *testing.T) {

I like the consistency of the data source having a "basic" test, but, it's up to you.

To help understand the naming rationale here, I'll use a generic example. Data sources tend to have varying argument requirements that change over time, so defining what a "basic" test might be over time could be challenging. 😄

Say a data source currently requires an ARN argument. Later it is updated to make ARN optional and add an optional name argument. Which is "basic" now? Is there a "basic" test? It would feel odd to me to not update the original test name to differentiate what its trying to test and potentially hard to catch that during reviews. Rather than have this problem later on, seems like we can solve for it upfront by naming tests based on data source arguments. I think the only guaranteed "basic" case for a data source is when it can accept no arguments and it might be odd to name the test "_NoArguments" or something. Maybe it would not be odd though since its more explicit.

To be fair, we also have this problem for resources and "basic" tests being for "all required arguments", it is just much less likely to happen over time. I'll leave myself a note to update the acceptance testing documentation in some fashion for this topic, since its not defined well. Relatedly, at some point we need to make decisions on #13128, which may or may not be defined by the documentation update we can collaborate on. 😉

If you feel strongly that this should be defined in a specific manner, let's discuss further in a new issue or in the change request for the documentation. There is also #8223 floating around out there, which wouldn't necessarily cover these naming situations completely, but could materially change the problem.

YakDriver · 2021-01-26T21:06:06Z

aws/resource_aws_imagebuilder_image_test.go

+ }
+
+ if err != nil {
+ return fmt.Errorf("error getting Image Builder Image (%s): %w", rs.Primary.ID, err)


Suggested change

return fmt.Errorf("error getting Image Builder Image (%s): %w", rs.Primary.ID, err)

return fmt.Errorf("unable to get Image Builder Image (%s): %w", rs.Primary.ID, err)

Nit. This would show up as Error: error getting... which seems redundant.

Thank you for raising this. Rather than leaving a lengthy comment that could be lost about the history of this, I have created #17314 with all the information. What is important right now is that fmt.Errorf("error creating/reading/updating/deleting Service Thing (ID): Error") is the (finally now 😖 ) documented practice for consistency that's been the goal last few years, which is showing its age in Terraform 0.12 and later. I would rather continue the existing practice for later find/replace-ability until we can decide as a team on any potential new standards.

Please feel free to check that out issue and contribute!

YakDriver · 2021-01-26T21:06:39Z

aws/resource_aws_imagebuilder_image_test.go

+ _, err := conn.GetImage(input)
+
+ if err != nil {
+ return fmt.Errorf("error getting Image Builder Image (%s): %w", rs.Primary.ID, err)


Suggested change

return fmt.Errorf("error getting Image Builder Image (%s): %w", rs.Primary.ID, err)

return fmt.Errorf("unable to get Image Builder Image (%s): %w", rs.Primary.ID, err)

YakDriver · 2021-01-26T21:11:46Z

aws/data_source_aws_imagebuilder_image.go

+ output, err := conn.GetImage(input)
+
+ if err != nil {
+ return fmt.Errorf("error getting Image Builder Image: %w", err)


Suggested change

return fmt.Errorf("error getting Image Builder Image: %w", err)

return fmt.Errorf("getting Image Builder Image: %w", err)

Nit. error doesn't add anything since Errorf() adds Error:. I think either drop error or add more more description.

YakDriver · 2021-01-26T21:16:09Z

aws/resource_aws_imagebuilder_image.go

+ o, n := d.GetChange("tags")
+
+ if err := keyvaluetags.ImagebuilderUpdateTags(conn, d.Id(), o, n); err != nil {
+ return fmt.Errorf("error updating tags for Image Builder Image (%s): %w", d.Id(), err)


Suggested change

return fmt.Errorf("error updating tags for Image Builder Image (%s): %w", d.Id(), err)

return fmt.Errorf("updating tags for Image Builder Image (%s): %w", d.Id(), err)

YakDriver · 2021-01-26T21:16:54Z

aws/resource_aws_imagebuilder_image.go

+ }
+
+ if err != nil {
+ return fmt.Errorf("error deleting Image Builder Image (%s): %w", d.Id(), err)


Suggested change

return fmt.Errorf("error deleting Image Builder Image (%s): %w", d.Id(), err)

return fmt.Errorf("deleting Image Builder Image (%s): %w", d.Id(), err)

YakDriver · 2021-01-26T21:17:35Z

aws/resource_aws_imagebuilder_image_test.go

+func testSweepImageBuilderImages(region string) error {
+ client, err := sharedClientForRegion(region)
+ if err != nil {
+ return fmt.Errorf("error getting client: %w", err)


Suggested change

return fmt.Errorf("error getting client: %w", err)

return fmt.Errorf("getting client: %w", err)

YakDriver · 2021-01-26T21:18:13Z

aws/resource_aws_imagebuilder_image_test.go

+ err := r.Delete(d, client)
+
+ if err != nil {
+ sweeperErr := fmt.Errorf("error deleting Image Builder Image (%s): %w", arn, err)


Suggested change

sweeperErr := fmt.Errorf("error deleting Image Builder Image (%s): %w", arn, err)

sweeperErr := fmt.Errorf("deleting Image Builder Image (%s): %w", arn, err)

YakDriver · 2021-01-26T21:18:45Z

aws/resource_aws_imagebuilder_image_test.go

+ }
+
+ if err != nil {
+ sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("error listing Image Builder Images: %w", err))


Suggested change

sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("error listing Image Builder Images: %w", err))

sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("listing Image Builder Images: %w", err))

ghost · 2021-01-28T22:13:54Z

This has been released in version 3.26.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 · 2021-02-27T17:10:18Z

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!

bflad added new-resource Introduces a new resource. new-data-source Introduces a new data source. labels Dec 11, 2020

bflad requested a review from a team as a code owner December 11, 2020 05:43

This was referenced Dec 11, 2020

New Data Source: aws_imagebuilder_image #16377

Closed

New Resource: aws_imagebuilder_image #16375

Closed

Base automatically changed from master to main January 23, 2021 00:59

YakDriver self-assigned this Jan 26, 2021

YakDriver added the partition/aws-us-gov Pertains to the aws-us-gov partition. label Jan 26, 2021

YakDriver approved these changes Jan 26, 2021

View reviewed changes

Update CHANGELOG for #16710

0f583b5

bflad merged commit 65fea77 into main Jan 27, 2021

bflad deleted the f-aws_imagebuilder_image branch January 27, 2021 17:39

github-actions bot added this to the v3.26.0 milestone Jan 27, 2021

github-actions bot pushed a commit that referenced this pull request Jan 27, 2021

Update CHANGELOG.md for #16710

dfc7d13

bflad added a commit that referenced this pull request Jan 27, 2021

Fix CHANGELOG for #16710

9088ce1

ghost locked as resolved and limited conversation to collaborators Feb 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_imagebuilder_image #16710

New Resource: aws_imagebuilder_image #16710

bflad commented Dec 11, 2020

YakDriver left a comment

YakDriver Jan 26, 2021

YakDriver Jan 26, 2021

bflad Jan 27, 2021

YakDriver Jan 26, 2021

bflad Jan 27, 2021

YakDriver Jan 26, 2021

YakDriver Jan 26, 2021

YakDriver Jan 26, 2021

YakDriver Jan 26, 2021

YakDriver Jan 26, 2021

YakDriver Jan 26, 2021

YakDriver Jan 26, 2021

ghost commented Jan 28, 2021

ghost commented Feb 27, 2021

	func TestAccAwsImageBuilderImageDataSource_Arn_Aws(t *testing.T) {
	func TestAccAwsImageBuilderImageDataSource_basic(t *testing.T) {

	return fmt.Errorf("error getting Image Builder Image (%s): %w", rs.Primary.ID, err)
	return fmt.Errorf("unable to get Image Builder Image (%s): %w", rs.Primary.ID, err)

	return fmt.Errorf("error getting Image Builder Image: %w", err)
	return fmt.Errorf("getting Image Builder Image: %w", err)

	return fmt.Errorf("error updating tags for Image Builder Image (%s): %w", d.Id(), err)
	return fmt.Errorf("updating tags for Image Builder Image (%s): %w", d.Id(), err)

	return fmt.Errorf("error deleting Image Builder Image (%s): %w", d.Id(), err)
	return fmt.Errorf("deleting Image Builder Image (%s): %w", d.Id(), err)

	return fmt.Errorf("error getting client: %w", err)
	return fmt.Errorf("getting client: %w", err)

	sweeperErr := fmt.Errorf("error deleting Image Builder Image (%s): %w", arn, err)
	sweeperErr := fmt.Errorf("deleting Image Builder Image (%s): %w", arn, err)

	sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("error listing Image Builder Images: %w", err))
	sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("listing Image Builder Images: %w", err))

New Resource: aws_imagebuilder_image #16710

New Resource: aws_imagebuilder_image #16710

Conversation

bflad commented Dec 11, 2020

Community Note

YakDriver 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

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

ghost commented Jan 28, 2021

ghost commented Feb 27, 2021