Add support for aws_batch_compute_environment #1048

shibataka000 · 2017-07-04T07:30:09Z

Description

Add support for aws_batch_compute_environment to creates a AWS Batch compute environment.

Tests

$ make testacc TESTARGS='-run=TestAccAWSBatchComputeEnvironment'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSBatchComputeEnvironment -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2 (53.54s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithTags
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithTags (40.32s)
=== RUN   TestAccAWSBatchComputeEnvironment_createSpot
--- PASS: TestAccAWSBatchComputeEnvironment_createSpot (44.21s)
=== RUN   TestAccAWSBatchComputeEnvironment_createUnmanaged
--- PASS: TestAccAWSBatchComputeEnvironment_createUnmanaged (51.36s)
=== RUN   TestAccAWSBatchComputeEnvironment_updateMaxvCpus
--- PASS: TestAccAWSBatchComputeEnvironment_updateMaxvCpus (74.04s)
=== RUN   TestAccAWSBatchComputeEnvironment_updateInstanceType
--- PASS: TestAccAWSBatchComputeEnvironment_updateInstanceType (91.76s)
=== RUN   TestAccAWSBatchComputeEnvironment_updateComputeEnvironmentName
--- PASS: TestAccAWSBatchComputeEnvironment_updateComputeEnvironmentName (78.01s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithoutComputeResources
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithoutComputeResources (22.59s)
=== RUN   TestAccAWSBatchComputeEnvironment_createUnmanagedWithComputeResources
--- PASS: TestAccAWSBatchComputeEnvironment_createUnmanagedWithComputeResources (49.45s)
=== RUN   TestAccAWSBatchComputeEnvironment_createSpotWithoutBidPercentage
--- PASS: TestAccAWSBatchComputeEnvironment_createSpotWithoutBidPercentage (25.54s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	530.836s

ashinohara · 2017-07-07T15:45:52Z

Hello @shibataka000 are you also working on support for aws_batch_job_definition and aws_batch_job_queue? I am asking because I have an implementation of those resources that also includes aws_batch_compute_environment but it seems you have beaten me to the punch. I am happy to collaborate and provide my code for aws_batch_job_definition and aws_batch_job_queue.

shibataka000 · 2017-07-09T02:21:41Z

Hello @ashinohara .
I have not been working on support for aws_batch_job_definition and aws_batch_job_queue yet.
So I am happy if you create PullRequest for supporting for them after this PullRequrest will be merged or closed.
Thank you.

ashinohara · 2017-07-10T16:18:05Z

Sounds good to me @shibataka000. I will utilize your code for aws_batch_compute_environment once this gets merged and then submit my pull request for the 2 additional resources.

mathewpeterson · 2017-08-10T14:27:59Z

@shibataka000 Would you mind resolving the merge conflicts?

shibataka000 · 2017-08-11T03:52:43Z

@mathewpeterson I resolved the merge conflicts. Thank you.

mathewpeterson · 2017-08-11T14:36:02Z

@radeksimko Is this PR something you can review? We currently have a need for this and have to rely on breaking out of terraform with local-exec to run python scripts which has it's own set of issues. It also sounds like @ashinohara has functioning code for aws_batch_job_definition and aws_batch_job_queue so we could have a fairly complete solution around AWS Batch.

Ninir · 2017-08-17T21:28:01Z

Hey folks,

For a better review, @shibataka000 do you think you could open a PR for the vendoring of Batch Compute vendors, and keep this one for the resources implementation?

Will then review this as soon as it is done! (Smaller PRs are always of a good help for us 😄 )

Thanks for the effort there!

shibataka000 · 2017-08-20T03:26:28Z

@Ninir Thank you for your advice.
I open a PR #1457 for the vendoring of Batch Compute vendors.

Ninir · 2017-08-21T13:33:15Z

@shibataka000 Could you resolve conflicts here please?

Will make the review right after that! :)

shibataka000 · 2017-08-21T15:01:47Z

@Ninir I resolved conflicts. Thank you.

mathewpeterson · 2017-08-21T15:18:52Z

@ashinohara Are you able to get your PR's ready for aws_batch_job_definition and aws_batch_job_queue resources?

ashinohara · 2017-08-21T15:47:44Z

@mathewpeterson Yes, I can do that. Would you prefer that I branch from this branch since the aws_batch_job_definition and aws_batch_job_queue are dependent upon the aws_batch_compute_environment or should I wait until this PR is merged and then branch from master?

mathewpeterson · 2017-08-21T20:18:01Z

@ashinohara I think you could open a PR against master and github should be able to merge cleanly.

ashinohara · 2017-08-21T21:08:31Z

@mathewpeterson Unfortunately my tests for aws_batch_job_queue require creating a aws_batch_compute_environment so in order for me to run those tests I need this feature to be available. I currently have my own code for aws_batch_compute_environment but I would like to be able to test it with the code in this PR.

Ninir · 2017-08-21T21:18:31Z

Hey Folks,

@shibataka000 Thanks!
@ashinohara If you can, please wait for this review 😄 .

Will make the review in the coming days! :)

mathewpeterson · 2017-08-22T12:29:44Z

@ashinohara Yes, sorry for the confusion.

I was just trying lead to the question: Have you updated your code use shibataka000's aws_batch_compute_environment implementation?

Sorry for any confusion. I think it's best to listen to @Ninir and just wait for this PR to be reviewed.

Ninir

Hi @shibataka000

Just made a review about Batch Compute Environments.
Seems very good, just left some cosmetic comments :)

Ninir · 2017-08-22T15:59:04Z

aws/resource_aws_batch_compute_environment.go

+			"compute_environment_name": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,


We should validate the name structure, as per: http://docs.aws.amazon.com/batch/latest/APIReference/API_CreateComputeEnvironment.html#Batch-CreateComputeEnvironment-request-computeEnvironmentName

Ninir · 2017-08-22T15:59:38Z

aws/resource_aws_batch_compute_environment.go

+			},
+			"service_role": {
+				Type:     schema.TypeString,
+				Required: true,


This should be validated to check that this is an ARN

Ninir · 2017-08-22T16:00:12Z

aws/resource_aws_batch_compute_environment.go

+			"state": {
+				Type:         schema.TypeString,
+				Optional:     true,
+				ValidateFunc: validation.StringInSlice([]string{"ENABLED", "DISABLED"}, true),


Can you use constants in place of ENABLED & DISABLED?

Ninir · 2017-08-22T16:01:13Z

aws/resource_aws_batch_compute_environment.go

+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validation.StringInSlice([]string{"MANAGED", "UNMANAGED"}, true),


Same here: could you use constants?

Ninir · 2017-08-22T16:06:41Z

aws/resource_aws_batch_compute_environment.go

+							Required: true,
+							ForceNew: true,
+							Elem:     &schema.Schema{Type: schema.TypeString},
+							Set:      schema.HashString,


You can omit this as far as I know!

Ninir · 2017-08-22T16:33:11Z

website/docs/r/batch_compute_environment.html.markdown

+
+* `bid_percentage` - (Optional) The minimum percentage that a Spot Instance price must be when compared with the On-Demand price for that instance type before instances are launched. 
+  For example, if your bid percentage is 20%, then the Spot price must be below 20% of the current On-Demand price for that EC2 instance.
+  This parameter is required for SPOT compute environments.


I would put this line 116 too

Ninir · 2017-08-22T16:33:54Z

website/docs/r/batch_compute_environment.html.markdown

+* `ec2_key_pair` - (Optional) The EC2 key pair that is used for instances launched in the compute environment.
+* `image_id` - (Optional) The Amazon Machine Image (AMI) ID used for instances launched in the compute environment.
+* `instance_role` - (Required) The Amazon ECS instance role applied to Amazon EC2 instances in a compute environment.
+* `instance_type` - (Required) The instances types that may launched.


What do you think of A list of instance types that may be launched?

Ninir · 2017-08-22T16:34:29Z

website/docs/r/batch_compute_environment.html.markdown

+* `instance_type` - (Required) The instances types that may launched.
+* `max_vcpus` - (Required) The maximum number of EC2 vCPUs that an environment can reach.
+* `min_vcpus` - (Required) The minimum number of EC2 vCPUs that an environment should maintain.
+* `security_group_ids` - (Required) The EC2 security group that is associated with instances launched in the compute environment.


What do you think of A list of EC2 security group IDS that are associated with instances...?

Ninir · 2017-08-22T16:34:42Z

website/docs/r/batch_compute_environment.html.markdown

+* `min_vcpus` - (Required) The minimum number of EC2 vCPUs that an environment should maintain.
+* `security_group_ids` - (Required) The EC2 security group that is associated with instances launched in the compute environment.
+* `spot_iam_fleet_role` - (Optional) The Amazon Resource Name (ARN) of the Amazon EC2 Spot Fleet IAM role applied to a SPOT compute environment.
+  This parameter is required for SPOT compute environments.


Could you put this line 126?

Ninir · 2017-08-22T16:35:03Z

website/docs/r/batch_compute_environment.html.markdown

+* `security_group_ids` - (Required) The EC2 security group that is associated with instances launched in the compute environment.
+* `spot_iam_fleet_role` - (Optional) The Amazon Resource Name (ARN) of the Amazon EC2 Spot Fleet IAM role applied to a SPOT compute environment.
+  This parameter is required for SPOT compute environments.
+* `subnets` - (Required) The VPC subnets into which the compute resources are launched.


What do you think of A list of VPC subnets into which the compute resources are launched.?

shibataka000 · 2017-08-27T14:12:57Z

Thank you for your advice. I'm updating codes.

…ment name

…h change

… greater than 2 Because DescribeComputeEnvironments return only one compute environment if DescribeComputeEnvironmentInput contain only one name of compute environment

…esource_aws_batch_compute_environment.go Because I know compute_resource block has change

shibataka000 · 2017-08-29T14:42:09Z

@Ninir Thank you for your advice! I have fixed codes and documents except this.
If there are any other advice, please give me it.

Ninir

I just left a few more comments, but it starts getting into shape :)

Good job on that @shibataka000 ! 👍

Ninir · 2017-09-07T20:09:28Z

aws/resource_aws_batch_compute_environment.go

+)
+
+const (
+	MANAGED   = "MANAGED"


Sorry if my comment was misleading, I meant to prefer the use of constants from the SDK, as exposed here

Ninir · 2017-09-07T20:10:40Z

aws/resource_aws_batch_compute_environment.go

+	}
+}
+
+func isCorrentComputeEnvironmentName(i interface{}, k string) (s []string, es []error) {


I think there is a typo in Corrent 😄
Also, it would be better to put this function in validators.go, and rename it to validateBatchComputeEnvironmentName

Ninir · 2017-09-07T20:12:48Z

website/aws.erb

@@ -270,6 +270,15 @@
                    </ul>
                </li>

+                <li<%= sidebar_current("docs-aws-resource-batch-compute-environment") %>>


This should be docs-aws-resource-batch

Ninir · 2017-09-07T20:13:54Z

aws/resource_aws_batch_compute_environment.go

+	return
+}
+
+func isArnOfIamRole(i interface{}, k string) (s []string, es []error) {


Same as above, it would be better to put this function in validators.go, and rename it to validateBatchComputeEnvironmentIamRole

Ninir · 2017-09-07T20:14:02Z

aws/resource_aws_batch_compute_environment.go

+	return
+}
+
+func isArnOfIamInstanceProfile(i interface{}, k string) (s []string, es []error) {


Ninir · 2017-09-07T20:15:16Z

aws/resource_aws_batch_compute_environment.go

+							Type:         schema.TypeString,
+							Optional:     true,
+							ForceNew:     true,
+							ValidateFunc: isArnOfIamRole,


It would be better to use the validateArn function, which has more guards to validate ARNs. Thoughts?

Ninir · 2017-09-07T20:15:24Z

aws/resource_aws_batch_compute_environment.go

+			"service_role": {
+				Type:         schema.TypeString,
+				Required:     true,
+				ValidateFunc: isArnOfIamRole,


It would be better to use the validateArn function, which has more guards to validate ARNs. Thoughts?

Ninir · 2017-09-07T20:18:23Z

website/docs/r/batch_compute_environment.html.markdown

+  Creates a AWS Batch compute environment.
+---
+
+# aws\_batch\_compute\_environment


Those \ are not needed anymore, it is safe to remove them

shibataka000 · 2017-09-11T14:39:14Z

@Ninir Thank you for your advice! I really appreciate your help!
If there are any other advice, please give me it.

Ninir · 2017-09-18T11:48:56Z

aws/resource_aws_batch_compute_environment.go

+				"type":                computeResource.Type,
+			},
+		})
+	}


Last thing to do would be to move this to a flatten function I think :)
It would then allow us to manage optional fields more easily, and be consistent with other parts of the code.

Once done, 'will run the tests and merge if it's all ok :)

shibataka000 · 2017-09-19T14:58:53Z

@Ninir Thank you for your advice!

Last thing to do would be to move this to a flatten function I think :)

For example, You mean resource_aws_s3_bucket_notification.go#L402?
I think I did it. If I have some misunderstanding, please tell me that.

Ninir

Hi @shibataka000

This looks very good to me :)

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSBatchComputeEnvironment'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSBatchComputeEnvironment -timeout 120m
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2 (60.14s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithTags
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithTags (68.02s)
=== RUN   TestAccAWSBatchComputeEnvironment_createSpot
--- PASS: TestAccAWSBatchComputeEnvironment_createSpot (61.38s)
=== RUN   TestAccAWSBatchComputeEnvironment_createUnmanaged
--- PASS: TestAccAWSBatchComputeEnvironment_createUnmanaged (71.00s)
=== RUN   TestAccAWSBatchComputeEnvironment_updateMaxvCpus
--- PASS: TestAccAWSBatchComputeEnvironment_updateMaxvCpus (71.49s)
=== RUN   TestAccAWSBatchComputeEnvironment_updateInstanceType
--- PASS: TestAccAWSBatchComputeEnvironment_updateInstanceType (82.03s)
=== RUN   TestAccAWSBatchComputeEnvironment_updateComputeEnvironmentName
--- PASS: TestAccAWSBatchComputeEnvironment_updateComputeEnvironmentName (91.23s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithoutComputeResources
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithoutComputeResources (14.48s)
=== RUN   TestAccAWSBatchComputeEnvironment_createUnmanagedWithComputeResources
--- PASS: TestAccAWSBatchComputeEnvironment_createUnmanagedWithComputeResources (32.44s)
=== RUN   TestAccAWSBatchComputeEnvironment_createSpotWithoutBidPercentage
--- PASS: TestAccAWSBatchComputeEnvironment_createSpotWithoutBidPercentage (13.82s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithGoodComputeEnvironmentName
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithGoodComputeEnvironmentName (38.54s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithBadComputeEnvironmentName1
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithBadComputeEnvironmentName1 (1.83s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithBadComputeEnvironmentName2
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithBadComputeEnvironmentName2 (1.82s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithBadIamInstanceProfileArn
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithBadIamInstanceProfileArn (1.84s)
=== RUN   TestAccAWSBatchComputeEnvironment_createEc2WithBadIamRoleArn
--- PASS: TestAccAWSBatchComputeEnvironment_createEc2WithBadIamRoleArn (1.34s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	611.456s

Thanks a LOT for making those changes over the days, and have such reactivity, really appreciated :)

shibataka000 · 2017-09-20T13:45:44Z

@Ninir I really appreciate your help!

@ashinohara I'm sorry to have kept you waiting long time.
You can create PR for supporting for aws_batch_job_definition and aws_batch_job_queue .
Thank you.

ashinohara · 2017-09-20T16:19:53Z

No problem @shibataka000, I appreciate your contribution. I am updating my code to work with yours and hope to submit a PR soon.

* Add support for aws_batch_compute_environment. * Fix document about batch_compute_environment * Use const instead of literal in resource_aws_batch_compute_environment.go * Omit unnecessary parameter in resource_aws_batch_compute_environment.go * Validate compute environment name in resource_aws_batch_compute_environment.go * Write ComputeEnvironmentInput struct to log instead of comput environment name * Stop writing log about changing of each fields because of unnecessity * Use StateChangeConf to wait status of compute envrionment in AWS Batch change * Stop validate that number of compute environment in AWS Batch are not greater than 2 Because DescribeComputeEnvironments return only one compute environment if DescribeComputeEnvironmentInput contain only one name of compute environment * Validate format of ARN in resource_aws_batch_compute_environment.go * Delete unused testcase in resource_aws_batch_compute_environment_test.go * Stop checking that children of compute_resource block has change in resource_aws_batch_compute_environment.go Because I know compute_resource block has change * Fix document * Use of constants from the SDK * Use the validateArn function * Move validateBatchComputeEnvironmentName function to validators.go * Move codes reading compute_resources to a flatten function

ghost · 2020-04-11T17:17:10Z

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!

radeksimko added the new-resource Introduces a new resource. label Jul 5, 2017

ashinohara mentioned this pull request Jul 11, 2017

Support for AWS Batch hashicorp/terraform#12187

Closed

shibataka000 force-pushed the resource_batch_compute_environment branch 2 times, most recently from e8121db to 1389461 Compare August 11, 2017 03:17

shibataka000 force-pushed the resource_batch_compute_environment branch from 1389461 to 0ed8511 Compare August 17, 2017 12:28

Ninir added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 17, 2017

shibataka000 mentioned this pull request Aug 20, 2017

Add AWS SDK for Batch to vendor #1457

Merged

This was referenced Aug 21, 2017

Extract AWS Batch Terraform module into separate repo wellcomecollection/platform#793

Closed

Move to define AWS Batch resources in pure TF wellcomecollection/platform#821

Closed

Add support for aws_batch_compute_environment.

5268ced

shibataka000 force-pushed the resource_batch_compute_environment branch from 0ed8511 to 5268ced Compare August 21, 2017 14:55

Ninir removed the waiting-response Maintainers are waiting on response from community or contributor. label Aug 21, 2017

Ninir suggested changes Aug 22, 2017

View reviewed changes

shibataka000 added 7 commits August 29, 2017 22:53

Write ComputeEnvironmentInput struct to log instead of comput environ…

1646dfe

…ment name

Stop writing log about changing of each fields because of unnecessity

6a4a03d

Use StateChangeConf to wait status of compute envrionment in AWS Batc…

216c733

…h change

Validate format of ARN in resource_aws_batch_compute_environment.go

b2e8115

Stop validate that number of compute environment in AWS Batch are not…

bbb09b1

… greater than 2 Because DescribeComputeEnvironments return only one compute environment if DescribeComputeEnvironmentInput contain only one name of compute environment

Delete unused testcase in resource_aws_batch_compute_environment_test.go

15ebd84

Stop checking that children of compute_resource block has change in r…

b7e9f87

…esource_aws_batch_compute_environment.go Because I know compute_resource block has change

shibataka000 force-pushed the resource_batch_compute_environment branch from 31c591f to b7e9f87 Compare August 29, 2017 13:53

Ninir reviewed Sep 7, 2017

View reviewed changes

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

shibataka000 added 3 commits September 11, 2017 23:12

Fix document

e9672f2

Use of constants from the SDK

a494850

Use the validateArn function

ec89dc0

shibataka000 force-pushed the resource_batch_compute_environment branch from 33d23fd to 0e3ed4d Compare September 11, 2017 14:43

Move validateBatchComputeEnvironmentName function to validators.go

a459665

shibataka000 force-pushed the resource_batch_compute_environment branch from 0e3ed4d to a459665 Compare September 11, 2017 14:49

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

Ninir reviewed Sep 18, 2017

View reviewed changes

Move codes reading compute_resources to a flatten function

73180b4

Ninir approved these changes Sep 19, 2017

View reviewed changes

Ninir merged commit 398dc65 into hashicorp:master Sep 19, 2017

Ninir mentioned this pull request Sep 19, 2017

Add support for AWS Batch service #1641

Closed

ghost locked and limited conversation to collaborators Apr 11, 2020

Add support for aws_batch_compute_environment #1048

Add support for aws_batch_compute_environment #1048

Conversation

shibataka000 commented Jul 4, 2017

Description

Tests

ashinohara commented Jul 7, 2017

shibataka000 commented Jul 9, 2017

ashinohara commented Jul 10, 2017

mathewpeterson commented Aug 10, 2017

shibataka000 commented Aug 11, 2017

mathewpeterson commented Aug 11, 2017

Ninir commented Aug 17, 2017

shibataka000 commented Aug 20, 2017 • edited Loading

Ninir commented Aug 21, 2017

shibataka000 commented Aug 21, 2017

mathewpeterson commented Aug 21, 2017

ashinohara commented Aug 21, 2017

mathewpeterson commented Aug 21, 2017

ashinohara commented Aug 21, 2017

Ninir commented Aug 21, 2017

mathewpeterson commented Aug 22, 2017

Ninir 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

shibataka000 commented Aug 27, 2017

shibataka000 commented Aug 29, 2017

Ninir 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

shibataka000 commented Sep 11, 2017

Choose a reason for hiding this comment

shibataka000 commented Sep 19, 2017

Ninir left a comment

Choose a reason for hiding this comment

shibataka000 commented Sep 20, 2017

ashinohara commented Sep 20, 2017

ghost commented Apr 11, 2020

shibataka000 commented Aug 20, 2017 •

edited

Loading