Allow using multiple lines of before and after userdata

cloudposse / terraform-aws-eks-node-group

Terraform module to provision a fully managed AWS EKS Node Group

https://cloudposse.com/accelerate

Apache License 2.0

90 stars 128 forks source link

Allow using multiple lines of before and after userdata #124

Closed jchanam closed 1 year ago

jchanam commented 2 years ago

what

Allow using multiple lines of before and after userdata

why

As the bash commands are added to a file, I don't see why they have to be only one line.

references

closes #123

Gowiem commented 2 years ago

cc @Nuru since this validation was originally implemented in #84

See @jchanam's thread in Slack regarding this issue here: https://sweetops.slack.com/archives/CB6GHNLG0/p1658405157443729

jchanam commented 2 years ago

@Gowiem @Nuru Do you think we could move forward with this PR?

jchanam commented 2 years ago

@aknysh What work and testing do you need to complete the PR?

The links you've sent refer to the master status of the two lines that I've changed. I don't understand what do you need me to see there. Could you be more specific?

aknysh commented 2 years ago

@aknysh What work and testing do you need to complete the PR?

The links you've sent refer to the master status of the two lines that I've changed. I don't understand what do you need me to see there. Could you be more specific?

sorry, wrong message, you are correct you updated the lines

aknysh commented 2 years ago

@aknysh What work and testing do you need to complete the PR? The links you've sent refer to the master status of the two lines that I've changed. I don't understand what do you need me to see there. Could you be more specific?

sorry, wrong message, you are correct you updated the lines

What I wanted to point out is the example needs to be updated https://github.com/cloudposse/terraform-aws-eks-node-group/blob/master/examples/complete/fixtures.us-east-2.tfvars#L35 (this is not related to your changes, the example uses a string but the var is a list of strings)

Nuru commented 2 years ago

@aknysh What work and testing do you need to complete the PR? The links you've sent refer to the master status of the two lines that I've changed. I don't understand what do you need me to see there. Could you be more specific?

sorry, wrong message, you are correct you updated the lines

What I wanted to point out is the example needs to be updated https://github.com/cloudposse/terraform-aws-eks-node-group/blob/master/examples/complete/fixtures.us-east-2.tfvars#L35 (this is not related to your changes, the example uses a string but the var is a list of strings)

@aknysh Although it may seem a bit awkward, the example works as intended because the variable is passed as the sole member of a list.

Nuru commented 2 years ago

@jchanam I am hesitant to approve this PR because, while it resolved the confusion you experienced, I worry that it will add confusion to someone else. Please see my comment at https://github.com/cloudposse/terraform-aws-eks-node-group/issues/123#issuecomment-1218462942 and let me know what you think.

Nuru commented 1 year ago

Closing this as stale, wontfix