-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Add support for aws_batch_compute_environment #1048
Add support for aws_batch_compute_environment #1048
Conversation
Hello @shibataka000 are you also working on support for |
Hello @ashinohara . |
Sounds good to me @shibataka000. I will utilize your code for |
@shibataka000 Would you mind resolving the merge conflicts? |
e8121db
to
1389461
Compare
@mathewpeterson I resolved the merge conflicts. Thank you. |
@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 |
1389461
to
0ed8511
Compare
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 Could you resolve conflicts here please? Will make the review right after that! :) |
0ed8511
to
5268ced
Compare
@Ninir I resolved conflicts. Thank you. |
@ashinohara Are you able to get your PR's ready for |
@mathewpeterson Yes, I can do that. Would you prefer that I branch from this branch since the |
@ashinohara I think you could open a PR against master and github should be able to merge cleanly. |
@mathewpeterson Unfortunately my tests for |
Hey Folks, @shibataka000 Thanks! Will make the review in the coming days! :) |
@ashinohara Yes, sorry for the confusion. I was just trying lead to the question: Have you updated your code use shibataka000's Sorry for any confusion. I think it's best to listen to @Ninir and just wait for this PR to be reviewed. |
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.
Just made a review about Batch Compute Environments.
Seems very good, just left some cosmetic comments :)
"compute_environment_name": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: 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 should validate the name structure, as per: http://docs.aws.amazon.com/batch/latest/APIReference/API_CreateComputeEnvironment.html#Batch-CreateComputeEnvironment-request-computeEnvironmentName
}, | ||
"service_role": { | ||
Type: schema.TypeString, | ||
Required: 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.
This should be validated to check that this is an ARN
"state": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ValidateFunc: validation.StringInSlice([]string{"ENABLED", "DISABLED"}, 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.
Can you use constants in place of ENABLED & DISABLED?
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: validation.StringInSlice([]string{"MANAGED", "UNMANAGED"}, 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.
Same here: could you use constants?
Required: true, | ||
ForceNew: true, | ||
Elem: &schema.Schema{Type: schema.TypeString}, | ||
Set: schema.HashString, |
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.
You can omit this as far as I know!
|
||
* `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. |
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 would put this line 116 too
* `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. |
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.
What do you think of A list of instance types that may be launched
?
* `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. |
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.
What do you think of A list of EC2 security group IDS that are associated with instances...
?
* `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. |
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.
Could you put this line 126?
* `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. |
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.
What do you think of A list of VPC subnets into which the compute resources are launched.
?
Thank you for your advice. I'm updating codes. |
… 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
31c591f
to
b7e9f87
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.
I just left a few more comments, but it starts getting into shape :)
Good job on that @shibataka000 ! 👍
) | ||
|
||
const ( | ||
MANAGED = "MANAGED" |
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.
Sorry if my comment was misleading, I meant to prefer the use of constants from the SDK, as exposed here
} | ||
} | ||
|
||
func isCorrentComputeEnvironmentName(i interface{}, k string) (s []string, es []error) { |
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 think there is a typo in Corrent 😄
Also, it would be better to put this function in validators.go, and rename it to validateBatchComputeEnvironmentName
website/aws.erb
Outdated
@@ -270,6 +270,15 @@ | |||
</ul> | |||
</li> | |||
|
|||
<li<%= sidebar_current("docs-aws-resource-batch-compute-environment") %>> |
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 should be docs-aws-resource-batch
return | ||
} | ||
|
||
func isArnOfIamRole(i interface{}, k string) (s []string, es []error) { |
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.
Same as above, it would be better to put this function in validators.go, and rename it to validateBatchComputeEnvironmentIamRole
return | ||
} | ||
|
||
func isArnOfIamInstanceProfile(i interface{}, k string) (s []string, es []error) { |
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.
Here also!
Type: schema.TypeString, | ||
Optional: true, | ||
ForceNew: true, | ||
ValidateFunc: isArnOfIamRole, |
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.
It would be better to use the validateArn function, which has more guards to validate ARNs. Thoughts?
"service_role": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
ValidateFunc: isArnOfIamRole, |
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.
It would be better to use the validateArn function, which has more guards to validate ARNs. Thoughts?
Creates a AWS Batch compute environment. | ||
--- | ||
|
||
# aws\_batch\_compute\_environment |
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.
Those \
are not needed anymore, it is safe to remove them
@Ninir Thank you for your advice! I really appreciate your help! |
33d23fd
to
0e3ed4d
Compare
0e3ed4d
to
a459665
Compare
"type": computeResource.Type, | ||
}, | ||
}) | ||
} |
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.
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 :)
@Ninir Thank you for your advice!
For example, You mean resource_aws_s3_bucket_notification.go#L402? |
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 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 :)
@Ninir I really appreciate your help! @ashinohara I'm sorry to have kept you waiting long time. |
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
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! |
Description
Add support for
aws_batch_compute_environment
to creates a AWS Batch compute environment.Tests