terraform-aws-eks-node-group icon indicating copy to clipboard operation
terraform-aws-eks-node-group copied to clipboard

Published 20 hours ago • verified publisher icon cloudposse

Test

Open Darsh8790 opened this issue 1 year ago • 4 comments

what

why

references

Darsh8790 avatar May 10 '24 09:05 Darsh8790

💥 This pull request now has conflicts. Could you fix it @Darsh8790? 🙏

mergify[bot] avatar May 10 '24 09:05 mergify[bot]

@Darsh8790 Thank you for this PR. However, as it is, it is not acceptable.

  1. Do not change default values. That would be a breaking change, which we try to avoid.
  2. I believe your regex change would break AL2 support. If it does not, please explain why in comments.
  3. If we are going to add support for AL2023, then we want to add support for both x86_64 and arm64 architectures.
  4. After making changes but before your final commit and push, please run:
make init
make precommit/terraform # requires `docker run` be available
  1. Please fill in the required PR comment fields.

Nuru avatar May 10 '24 20:05 Nuru

@Darsh8790 Note that you will need to provide backward compatibility for AL2 users, so you cannot just replace the existing userdata.tpl with one for AL2023. Probably you should use the same inputs and have different templates for AL2 and AL2023, like the way we have different templates for AL2 and Windows.

Also, I don't know why there are merge conflicts, but you should probably resolve them by rebasing your changes on main.

Nuru avatar May 19 '24 23:05 Nuru

Reviewers please note

  • Extra configuration for AL2 is done by passing arguments to bootstrap.sh.
  • For AL2023, configuration is handled via nodeadm.

In order to support AL2023, it is not only the AMI selection that needs to be updated, but also everything configured via userdata.

Nuru avatar May 19 '24 23:05 Nuru

This PR was closed due to inactivity and merge conflicts. 😭 Please resolve the conflicts and reopen if necessary.

mergify[bot] avatar Jun 09 '24 09:06 mergify[bot]