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.

Sign up for GitHub

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

Jump to bottom

Add import support for aws_batch_job_definition #11407

Merged
merged 4 commits into from
Feb 6, 2020
Merged

Add import support for aws_batch_job_definition #11407

merged 4 commits into from
Feb 6, 2020

Conversation

prabusah
Copy link
Contributor

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 #11207

Release note for CHANGELOG:

resource/aws_batch_job_definition: support resource import

Output from acceptance testing:

Did not run acceptance testing

...

@prabusah prabusah requested a review from a team December 22, 2019 05:55
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/batch Issues and PRs that pertain to the batch service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. needs-triage Waiting for first response or review from a maintainer. labels Dec 22, 2019
@prabusah prabusah changed the title 11207 import aws batch job definition Add import support for aws_batch_job_definition Dec 22, 2019
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Jan 2, 2020
bflad
bflad reviewed Jan 2, 2020
View reviewed changes
aws/resource_aws_batch_job_queue.go Outdated
@@ -19,6 +19,13 @@ func resourceAwsBatchJobQueue() *schema.Resource {
Update: resourceAwsBatchJobQueueUpdate,
Delete: resourceAwsBatchJobQueueDelete,

Importer: &schema.ResourceImporter{
Copy link
Contributor

@bflad bflad Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please note: This pull request currently contains the changes from #11406, which needs adjustments. If the aws_batch_job_queue changes are removed from this pull request, we can likely get the aws_batch_job_definition changes (which look okay at first glance) merged sooner. 👍

@bflad bflad self-assigned this Jan 6, 2020
@bflad bflad modified the milestone: v2.44.0 Jan 6, 2020
bflad
bflad suggested changes Jan 6, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @prabusah 👋 Please note this pull request is dependent on the fixes for the container_properties attribute in #11488, otherwise the testing fails, e.g.

--- FAIL: TestAccAWSBatchJobDefinition_updateForcesNewResource (28.38s)
    testing.go:640: Step 2 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) {
        }


        (map[string]string) (len=2) {
         (string) (len=20) "container_properties": (string) (len=317) "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":1024,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}",
         (string) (len=4) "name": (string) (len=50) "tf_acctest_batch_job_definition_702790761299881205"
        }

In the real world, this test failure indicates that importing this resource would always show a difference on that attribute on the next Terraform run. Since the container_properties attribute is ForceNew: true, this would be unexpectedly destructive.

In the meantime, can you please remove the aws_batch_job_queue resource changes in the pull request (either via new commit and/or rebasing)? Thanks.

bflad added a commit that referenced this pull request Jan 6, 2020
@bflad
resource/aws_batch_job_definition: Properly read name into Terraform …
7c2cb1c 
…state

Reference: #11407 (review)

To prevent future issues with resource import, when implemented.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.00s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (27.64s)
```
bflad added a commit that referenced this pull request Feb 5, 2020
@bflad
resource/aws_batch_job_definition: Properly set container_properties …
0a8efb1 
…and name into Terraform state and fix basic test (#11488)

* tests/resource/aws_batch_job_definition: Fix TestAccAWSBatchJobDefinition_basic ContainerProperties comparison to match updated API response

Previously:

```
--- FAIL: TestAccAWSBatchJobDefinition_basic (5.36s)
    testing.go:640: Step 0 error: Check failed: Check 2/2 error: Bad Job Definition Container Properties
        	 expected: {
          Command: ["ls","-la"],
          Environment: [{
              Name: "VARNAME",
              Value: "VARVAL"
            }],
          Image: "busybox",
          Memory: 512,
          MountPoints: [{
              ContainerPath: "/tmp",
              ReadOnly: false,
              SourceVolume: "tmp"
            }],
          Ulimits: [{
              HardLimit: 1024,
              Name: "nofile",
              SoftLimit: 1024
            }],
          Vcpus: 1,
          Volumes: [{
              Host: {
                SourcePath: "/tmp"
              },
              Name: "tmp"
            }]
        }
        	got: {
          Command: ["ls","-la"],
          Environment: [{
              Name: "VARNAME",
              Value: "VARVAL"
            }],
          Image: "busybox",
          Memory: 512,
          MountPoints: [{
              ContainerPath: "/tmp",
              ReadOnly: false,
              SourceVolume: "tmp"
            }],
          ResourceRequirements: [],
          Ulimits: [{
              HardLimit: 1024,
              Name: "nofile",
              SoftLimit: 1024
            }],
          Vcpus: 1,
          Volumes: [{
              Host: {
                SourcePath: "/tmp"
              },
              Name: "tmp"
            }]
        }
```

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (15.23s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (26.75s)
```

* resource/aws_batch_job_definition: Properly set container_properties into Terraform state and perform drift detection

Reference: #9954
Reference: #11038

Previously, we were silently ignoring the `d.Set()` error for the `container_properties` attribute. This error can be seen with adding error checking on the call or with `tfproviderlint -R004`:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_batch_job_definition.go:146:32: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/batch.ContainerProperties
```

Here we introduce the conversion the Batch `ContainerProperties` object into a JSON string, similar to the handling of ECS `ContainerDefinitions`. With the new attribute properly setting into the Terraform state, existing tests were failing with differences now being discovered:

```
--- FAIL: TestAccAWSBatchJobDefinition_basic (12.38s)
    testing.go:640: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        DESTROY/CREATE: aws_batch_job_definition.test
          ... omitted for clarity ...
          container_properties:               "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"resourceRequirements\":[],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" => "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" (forces new resource)
          ... omitted for clarity ...
```

Similar to some fields in ECS `ContainerDefinitions`, the API will inject an empty JSON array at least in the `RequiredResources` field. Instead of burdening operators with exactly matching the canonical API JSON, we insert difference suppression for this case. We also suppress reordered `Environment` objects (by `Name`; similar to ECS), since it is likely feature request as well.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.45s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (24.61s)
```

* resource/aws_batch_job_definition: Properly read name into Terraform state

Reference: #11407 (review)

To prevent future issues with resource import, when implemented.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.00s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (27.64s)
```
@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Feb 6, 2020
@bflad bflad added this to the v2.48.0 milestone Feb 6, 2020
bflad
bflad approved these changes Feb 6, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Rebased on master to fix merge conflicts. Now that the fixes from #11488 are merged and after re-applying testing fix of #11406 (comment), this looks good. Thanks @prabusah 🚀

Output from acceptance testing:

--- PASS: TestAccAWSBatchJobDefinition_basic (16.49s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (28.51s)

@bflad bflad merged commit 30380f9 into hashicorp:master Feb 6, 2020
bflad added a commit that referenced this pull request Feb 6, 2020
@bflad
Update CHANGELOG for #11407
18b6e4f
@ghost
Copy link

ghost commented Feb 7, 2020

This has been released in version 2.48.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 Mar 27, 2020

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 and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees

@bflad bflad

Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/batch Issues and PRs that pertain to the batch service. size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v2.48.0
Development

Successfully merging this pull request may close these issues.

Support for importing AWS Batch resource types
2 participants
@prabusah @bflad