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

feat: migrates example on eks-cluster-aws-4.x #173

Closed gberenice closed 6 months ago

gberenice commented 6 months ago

what

why

references

gberenice commented 6 months ago

These changes were released in v3.0.0.

gberenice commented 6 months ago

/terratest

joe-niland commented 6 months ago

Seems OK, as it's based on the referenced PR

gberenice commented 6 months ago

/terratest

Nuru commented 6 months ago

@joe-niland @gberenice Thank you for this PR.

For future reference:

  1. Any PRs that do not change the code of the Terraform module itself should be labeled no-release to avoid triggering the release of a new module version upon merging the PR. Without this label, a new release may be created and people and systems feel the need to update to the new release, but the new release is exactly the same code, and it is just a waste of time and resources to upgrade. There are filters in place to try to catch PRs like this one that do not change any code and therefore do not trigger a release, and this PR was caught by the filter, but still, it is best practice to label the PR no-release to be on the safe side.

    Side note: you can also use no-release when you plan to merge several PRs in one day, so that you can batch the changes and reduce the upgrade noise.

  2. PRs that do change the code, but only make small changes, should get patch or bugfix labels. I mention it because although this PR should have had the no-release label, failing that, it should have had the patch label.

  3. Also, please note that any change to the Kubernetes version in the example requires a corresponding change to the Kubernetes Go packages in test/src/go.mod when such packages are present: https://github.com/cloudposse/terraform-aws-eks-node-group/blob/fa3d05a8cdb771ea1376775334ca67252970ea70/test/src/go.mod#L9-L10

    And then, of course, changing the go.mod file requires updating the go.sum file with go mod tidy.

See #177 for example.

https://github.com/cloudposse/terraform-aws-eks-node-group/blob/59e4145da6c28e05f4fe6b47f75af1b02257b728/test/src/go.mod#L11-L12

Specifically, the minor version numbers must match. Here K8s version 1.25.x used packages versions v0.25.x and the update to K8s v1.29.x requires updating the packages to v0.29.x. The patch levels do not need to match.

gberenice commented 6 months ago

Thanks for the explanation @Nuru. I feel like these important details should be described in the community docs, but I can't find this info there. Am I missing something?

Nuru commented 6 months ago

Thanks for the explanation @Nuru. I feel like these important details should be described in the community docs, but I can't find this info there. Am I missing something?

@gberenice You are right, they should be, but are not, documented there. I made a note to update the Code Review Guidelines.

joe-niland commented 6 months ago

@Nuru thanks for the notes. They are very helpful.

Agreed that it will be beneficial to document them.

It might also be helpful to update CONTRIBUTING because it covers labelling and updating of the examples.