r/emr_cluster: enable configuration of ebs root volume size

[rolandjohann]

EBS root volume size configuration implementation

[Ninir]

Hey @rmmjohann

This seems very good for a 1st-time contribution! 👍

Could you also check the attribute existence in the acceptance test?
It would be awesome to add another step in the test that updates the attribute, so that we know everything is ok.

If you need any help on that, feel free to ask!

[rolandjohann]

I'll do this within the next days

[rolandjohann]

@Ninir done

[Ninir]

Hi @rolandjohann

This looks good to me :)

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEMRCluster_'     
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEMRCluster_ -timeout 120m
=== RUN   TestAccAWSEMRCluster_basic
--- PASS: TestAccAWSEMRCluster_basic (622.78s)
=== RUN   TestAccAWSEMRCluster_instance_group
--- PASS: TestAccAWSEMRCluster_instance_group (722.08s)
=== RUN   TestAccAWSEMRCluster_security_config
--- PASS: TestAccAWSEMRCluster_security_config (648.09s)
=== RUN   TestAccAWSEMRCluster_bootstrap_ordering
--- PASS: TestAccAWSEMRCluster_bootstrap_ordering (672.08s)
=== RUN   TestAccAWSEMRCluster_visibleToAllUsers
--- PASS: TestAccAWSEMRCluster_visibleToAllUsers (1497.36s)
=== RUN   TestAccAWSEMRCluster_tags
--- PASS: TestAccAWSEMRCluster_tags (1146.55s)
=== RUN   TestAccAWSEMRCluster_basic
--- PASS: TestAccAWSEMRCluster_basic (689.31s)

This is just missing the documentation update, so that users know of this feature. Can you add it please?

Also and just for the information: do you have an official documentation link where I could see this attribute in action? can't find one in the EMR cluster API nor in the CloudFormation one, only in the CLI one.
Would like to see validation and so on.

Thanks!

[rolandjohann]

Sure, here are the docs: https://docs.aws.amazon.com/sdk-for-go/api/service/emr/#RunJobFlowInput.
Just saw that root volume size is supported by EMR 4.x upwards only. Will check that tomorrow.

Your test output doesn't contain the test for the root volume size. Is this the full output?

Yes, docs are crutial - didn't had them in mind. I'll fix them tomorrow as well.

[Ninir]

Your test output doesn't contain the test for the root volume size. Is this the full output?

It seems the output has been cut, but the test do passed :)

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEMRCluster_root_volume_size'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEMRCluster_root_volume_size -timeout 120m
=== RUN   TestAccAWSEMRCluster_root_volume_size
--- PASS: TestAccAWSEMRCluster_root_volume_size (1530.05s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1530.085s

Sure, here are the docs: https://docs.aws.amazon.com/sdk-for-go/api/service/emr/#RunJobFlowInput.
Just saw that root volume size is supported by EMR 4.x upwards only. Will check that tomorrow.

Yes, this was my concern when checking the docs: can't find anything about this particular attribute in the API / CF but in the CLI & SDK.

Yes, docs are crutial - didn't had them in mind. I'll fix them tomorrow as well.

Thank your for that! :)

[rolandjohann]

Strange, that there is no reference at the API and CloudFormation docs.

The terraform docs are now updated.

Setting ebs_root_volume_size when using EMR version below 4.x is a misconfiguration, and shouldn't be handled by terraform, IMO. Are there any conventions at terraform how to handle those errors?

[rolandjohann]

@Ninir is there somehting left to do?

[Ninir]

Hey @rolandjohann

Seems good to me :)

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEMRCluster_root_volume_size' 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEMRCluster_root_volume_size -timeout 120m
=== RUN   TestAccAWSEMRCluster_root_volume_size
--- PASS: TestAccAWSEMRCluster_root_volume_size (1204.53s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1204.569s

Thanks for the work! 👍

[ghost]

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!