Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow using multiple lines of before and after userdata #124

Closed
wants to merge 3 commits into from
Closed

Allow using multiple lines of before and after userdata #124

wants to merge 3 commits into from

Conversation

jchanam
Copy link

@jchanam jchanam commented Jul 21, 2022

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

@jchanam jchanam requested review from a team as code owners July 21, 2022 12:24
@jchanam jchanam requested review from Gowiem and RothAndrew July 21, 2022 12:24
@Gowiem
Copy link
Member

Gowiem commented Jul 21, 2022

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

@Gowiem Gowiem requested a review from Nuru July 21, 2022 21:42
@nitrocode nitrocode added the patch A minor, backward compatible change label Jul 24, 2022
@jchanam
Copy link
Author

jchanam commented Aug 17, 2022

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

aknysh
aknysh requested changes Aug 17, 2022
View reviewed changes
Copy link
Member

@aknysh aknysh left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jchanam thanks for the PR, it looks good to me, let @Nuru review it

@jchanam
Copy link
Author

jchanam commented Aug 17, 2022

@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
aknysh reviewed Aug 17, 2022
View reviewed changes
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let @Nuru review it

@aknysh
Copy link
Member

aknysh commented Aug 17, 2022 •
edited
Loading

@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
Copy link
Member

aknysh commented Aug 17, 2022 •
edited
Loading

@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
Copy link
Contributor

Nuru commented Aug 17, 2022

@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
Copy link
Contributor

Nuru commented Aug 17, 2022

@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 #123 (comment) and let me know what you think.

@Nuru Nuru added invalid This doesn't seem right do not merge Do not merge this PR, doing so would cause problems labels Aug 19, 2022
@Nuru
Copy link
Contributor

Nuru commented Oct 27, 2022

Closing this as stale, wontfix

@Nuru Nuru closed this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aknysh aknysh aknysh requested changes

@Gowiem Gowiem Awaiting requested review from Gowiem Gowiem was automatically assigned from cloudposse/contributors

@RothAndrew RothAndrew Awaiting requested review from RothAndrew RothAndrew was automatically assigned from cloudposse/contributors

@Nuru Nuru Awaiting requested review from Nuru

Assignees
No one assigned
Labels
do not merge Do not merge this PR, doing so would cause problems invalid This doesn't seem right patch A minor, backward compatible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

after_cluster_joining_userdata only allows one element
6 participants
@jchanam @Gowiem @aknysh @Nuru @nitrocode @cloudpossebot