search

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
91 stars 128 forks source link

Test #178

Closed Darsh8790 closed 5 months ago

Darsh8790 commented 6 months ago

what

why

references

mergify[bot] commented 6 months ago

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

Nuru commented 6 months ago

@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 commented 5 months ago

@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 commented 5 months ago

Reviewers please note

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

mergify[bot] commented 5 months ago

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