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.

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

instance-selector is now patched to work w/ EKS AMI V20220123 #4682

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

bwagner5
Copy link
Contributor

Description

ec2-instance-selector was unnecessarily validating that the version of the EKS AMI (https://github.com/awslabs/amazon-eks-ami/releases/tag/v20220123) was prepended with a lowercase v. But the latest release of the AMI had an uppercase V (has since been fixed). A patch was put into place in eksctl (#4681). The EC2 Instance Selector has since been patched (aws/amazon-ec2-instance-selector#112) and so the eksctl patch can be reverted and point to the latest commit of the official instance-selector repo.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@bwagner5 bwagner5 force-pushed the ec2-instance-selector branch from 1061923 to 1007461 Compare January 24, 2022 22:56
@bwagner5
Copy link
Contributor Author

Manual Testing:

$ make binary
$ ./eksctl create nodegroup --instance-selector-vcpus 2 --dry-run --cluster my-demo
apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig
managedNodeGroups:
- amiFamily: AmazonLinux2
  desiredCapacity: 2
  disableIMDSv1: false
  disablePodIMDS: false
  iam:
    withAddonPolicies:
      albIngress: false
      appMesh: false
      appMeshPreview: false
      autoScaler: false
      certManager: false
      cloudWatch: false
      ebs: false
      efs: false
      externalDNS: false
      fsx: false
      imageBuilder: false
      xRay: false
  instanceSelector:
    vCPUs: 2
  instanceTypes:
  - c4.large
  - c5.large
  - c5a.large
  - c5d.large
  - c5n.large
  - c6i.large
  - i3.large
  - i3en.large
 .....

@cPu1 cPu1 added the area/tech-debt Leftover improvements in code, testing and building label Jan 25, 2022
Copy link
Contributor

@cPu1 cPu1 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@cPu1 cPu1 enabled auto-merge (squash) January 25, 2022 07:50
@cPu1 cPu1 added the skip-release-notes Causes PR not to show in release notes label Jan 25, 2022
@cPu1 cPu1 merged commit 34a6331 into eksctl-io:main Jan 25, 2022
@patstrom
Copy link
Contributor

patstrom commented Jan 25, 2022

Cheers 🎉

Will this be released as a patch or will it be included in the next minor release (in ~2 weeks?)?

@cPu1
Copy link
Contributor

cPu1 commented Jan 25, 2022

Cheers tada

Will this be released as a patch or will it be included in the next minor release (in ~2 weeks?)?

You don't need to wait for a patch release. The EKS AMI repo has fixed its release tag, so your existing version of eksctl will now work.

@patstrom
Copy link
Contributor

You don't need to wait for a patch release. The EKS AMI repo has fixed its release tag, so your existing version of eksctl will now work.

Oh wow. Even better 🎉

@hspencer77 hspencer77 mentioned this pull request Jul 8, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tech-debt Leftover improvements in code, testing and building skip-release-notes Causes PR not to show in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants