Add support for batch_job_definition and batch_job_queue #1710

ashinohara · 2017-09-20T17:26:09Z

Description

Adding resources aws_batch_job_definition and aws_batch_job_queue.

Additionally, I found an issue with some of the tests for aws_batch_compute_environment which seems to be a condition where the aws_iam_role_policy_attachment gets deleted before the compute environment and causes the aws_batch_compute_environment to be a dangling resource. I have added a note in the documentation about it, and have also fixed the tests by adding a depends_on link in the aws_batch_compute_environment to the aws_iam_role_policy_attachment.

Tests

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSBatchJobDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSBatchJobDefinition -timeout 120m
=== RUN   TestAccAWSBatchJobDefinition
--- PASS: TestAccAWSBatchJobDefinition (14.26s)
=== RUN   TestAccAWSBatchJobDefinitionUpdateForcesNewResource
--- PASS: TestAccAWSBatchJobDefinitionUpdateForcesNewResource (22.15s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	36.442s
$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSBatchJobQueue'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSBatchJobQueue -timeout 120m
=== RUN   TestAccAWSBatchJobQueue
--- PASS: TestAccAWSBatchJobQueue (89.57s)
=== RUN   TestAccAWSBatchJobQueueUpdate
--- PASS: TestAccAWSBatchJobQueueUpdate (104.32s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	193.919s

radeksimko

Hi @ashinohara
thanks for crafting this!

I left you some comments in the code, some are more important than others, I hope they all make sense though.

Let me know if you have any questions.

The PR otherwise looks pretty good and fairly close to shippable state.

radeksimko · 2017-09-26T16:47:33Z

aws/resource_aws_batch_job_definition.go

+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,
+				StateFunc: func(v interface{}) string {


Did you add this StateFunc just to suppress the JSON formatting differences in plan? If so, then we have a little bit more elegant solution which doesn't hide the JSON from the plan:
https://github.com/terraform-providers/terraform-provider-aws/blob/90d698c66db80d676e2db197be97c1a9977e5393/aws/resource_aws_cloudwatch_dashboard.go#L36-L40

Whether the user wants to see full JSON in the plan is questionable, but it seems some folks find it useful and it would be nice to avoid breaking change later on, like here.

Yes, that was the intent. Thanks for pointing me to that function.

radeksimko · 2017-09-26T16:49:58Z

aws/resource_aws_batch_job_definition.go

+				Elem:     schema.TypeString,
+			},
+			"retry_strategy": {
+				Type:     schema.TypeSet,


Is there any particular reason this field should be TypeSet and not TypeList? We could make it easier to reference with TypeList, i.e. retry_strategy.0.attempts, instead of retry_strategy.<computed-hash>.attempts.

radeksimko · 2017-09-26T16:51:08Z

aws/resource_aws_batch_job_definition.go

+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,
+			},


Is there any reason why revision and arn should be Optional? I don't see them being used anywhere in the CRUD other than in Set() which makes me think they should be Computed-only... 🤔

radeksimko · 2017-09-26T17:01:48Z

aws/resource_aws_batch_job_definition.go

+	if err != nil {
+		return fmt.Errorf("%s %q", err, arn)
+	}
+	d.Set("arn", *job.JobDefinitionArn)


We tend to avoid dereferencing simple data types in this context as we burnt our fingers in the past by doing that - i.e. API returned empty field which then caused a crash due to nil dereferencing.

The Set function is capable of safe dereferencing, so we can just let it do the job. 😉
https://github.com/hashicorp/terraform/blob/d78b575536ddf862946521e03b3c57402e3201be/helper/schema/resource_data.go#L169-L185

TL;DR just pass the pointer.

radeksimko · 2017-09-26T17:03:02Z

aws/resource_aws_batch_job_definition.go

+	}
+	d.Set("arn", *job.JobDefinitionArn)
+	d.Set("container_properties", *job.ContainerProperties)
+	d.Set("parameters", job.Parameters)


I think we'll need to convert the map[string]*string to map[string]string here. Feel free to use the SDK helper aws.StringValueMap.

radeksimko · 2017-09-28T09:42:30Z

aws/resource_aws_batch_job_queue.go

+		Priority:                aws.Int64(int64(d.Get("priority").(int))),
+		State:                   aws.String(d.Get("state").(string)),
+	}
+	_, err := conn.UpdateJobQueue(updateInput)


Shouldn't we also wait here for the job until it goes from batch.JQStatusUpdating to batch.JQStatusValid?

radeksimko · 2017-09-28T09:44:43Z

aws/resource_aws_batch_job_queue.go

+	return nil
+}
+
+func createComputeEnvironmentOrder(d *schema.ResourceData) (envs []*batch.ComputeEnvironmentOrder) {


Maybe a nitpick, but the interface here seems to be quite greedy - any reason we can't just pass an []interface{} and call d.Get() outside of this function since it only needs access to a single field, not all ResourceData?

radeksimko · 2017-09-28T09:46:48Z

aws/resource_aws_batch_job_queue.go

+			return nil
+		}
+		return resource.RetryableError(err)
+	})


Is there any particular reason why this should fail and/or why we should retry? Can we retry on a specific error code, instead of any error?

Also if we really need the retry logic, then we should also catch the err coming from it - right now it seems to be shadowed on line 152.

I was using it to wait until the Job Queue was disabled, but I think a more elegant solution is to use WaitForState. I am working on that change.

radeksimko · 2017-09-28T09:56:34Z

aws/validators.go

 	value := v.(string)
 	if !(regexp.MustCompile(`^[A-Za-z0-9_]*$`).MatchString(value) && len(value) <= 128) {
-		errors = append(errors, fmt.Errorf("computeEnvironmentName must be up to 128 letters (uppercase and lowercase), numbers, and underscores."))
+		errors = append(errors, fmt.Errorf("Name must be up to 128 letters (uppercase and lowercase), numbers, and underscores."))


I have raised https://github.com/terraform-providers/terraform-provider-aws/pull/1760/files to address the generic-ness of this function - I agree it can be still renamed, but either way - do you mind rebasing?

radeksimko · 2017-09-28T09:57:25Z

website/docs/r/batch_job_definition.html.markdown

+
+```hcl
+resource "aws_batch_job_definition" "test" {
+	name = "tf-test-batch-job-definition"


I think we should replace hyphens with underscores here, per validation function.

ashinohara · 2017-09-28T16:31:16Z

@radeksimko Thank you very much for your insightful review. I believe I have addressed all of your comments and made sure that tests are still passing. Let me know if there is anything else you would like me to change.

radeksimko

Thanks for addressing my comments.

radeksimko · 2017-09-28T16:39:02Z

aws/resource_aws_batch_job_definition.go

+			},
+			"revision": {
+				Type:     schema.TypeInt,
+				Optional: true,


The revision here is still Optional, but I will take care of that prior to merging.

pat-nomalab · 2017-09-29T12:35:02Z

Hi,
thanks for this feature
I've managed to create batch compute env with job queue link to it, but I have an issue when i'm trying to upgrade compute env definition "Cannot delete, found existing JobQueue relationship"

mkubenka

@pat-nomalab Check my comment. With that change destroy will work correctly.

mkubenka · 2017-10-04T15:55:33Z

aws/resource_aws_batch_job_queue.go

+	_, err = conn.DeleteJobQueue(&batch.DeleteJobQueueInput{
+		JobQueue: aws.String(sn),
+	})
+	if err == nil {


@ashinohara This is probably bug and it should be:

if err !=nil { return err }

Otherwise it's not possible to run destroy because compute environment couldn't be deleted with JobQueue defined.

ghost · 2020-04-11T17:12:25Z

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!

ashinohara mentioned this pull request Sep 20, 2017

Add support for AWS Batch service #1641

Closed

radeksimko added enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. labels Sep 25, 2017

radeksimko reviewed Sep 28, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 28, 2017

Andrew Shinohara added 3 commits September 28, 2017 08:22

Adding batch_job_definition and batch_job_queue resources

4ad1244

FIxing issues found by go vet

4ed4351

Addressing pull request comments

7bb5184

ashinohara force-pushed the resource_aws_batch branch from ce4a18a to 7bb5184 Compare September 28, 2017 16:28

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 28, 2017

radeksimko approved these changes Sep 28, 2017

View reviewed changes

radeksimko added 2 commits September 28, 2017 17:45

Make revision Computed-only field

868974e

Update test names to meet convention

b1bef6f

radeksimko merged commit 435f1c9 into hashicorp:master Sep 28, 2017

mkubenka reviewed Oct 4, 2017

View reviewed changes

This was referenced Nov 16, 2017

Fix bug that not waiting batch_job_queue deleted. #2322

Merged

Cannot delete error occurs when aws_batch_compute_environment used in aws_batch_job_queue is recreated #2044

Closed

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for batch_job_definition and batch_job_queue #1710

Add support for batch_job_definition and batch_job_queue #1710

ashinohara commented Sep 20, 2017

radeksimko left a comment

radeksimko Sep 26, 2017

ashinohara Sep 28, 2017

radeksimko Sep 26, 2017

radeksimko Sep 26, 2017

radeksimko Sep 26, 2017

radeksimko Sep 26, 2017

radeksimko Sep 28, 2017

radeksimko Sep 28, 2017

radeksimko Sep 28, 2017

radeksimko Sep 28, 2017

ashinohara Sep 28, 2017

radeksimko Sep 28, 2017

radeksimko Sep 28, 2017

ashinohara commented Sep 28, 2017

radeksimko left a comment

radeksimko Sep 28, 2017

pat-nomalab commented Sep 29, 2017

mkubenka left a comment

mkubenka Oct 4, 2017

ghost commented Apr 11, 2020

Add support for batch_job_definition and batch_job_queue #1710

Add support for batch_job_definition and batch_job_queue #1710

Conversation

ashinohara commented Sep 20, 2017

Description

Tests

radeksimko 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

Choose a reason for hiding this comment

ashinohara commented Sep 28, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pat-nomalab commented Sep 29, 2017

mkubenka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 11, 2020