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

Fix issue #709 #710

Merged
merged 2 commits into from
Jun 8, 2023
Merged

Fix issue #709 #710

merged 2 commits into from
Jun 8, 2023

Conversation

bnaydenov
Copy link
Contributor

Fix #709

Copy link
Collaborator

@youngjeong46 youngjeong46 left a comment

Choose a reason for hiding this comment

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

Thank you @bnaydenov. Left a couple of comments.

lib/addons/cluster-autoscaler/index.ts Show resolved Hide resolved
@@ -42,8 +42,9 @@ const defaultProps: ClusterAutoScalerAddOnProps = {
* Version of the autoscaler, controls the image tag
*/
const versionMap = new Map([
[KubernetesVersion.of("1.25"), "9.25.0"],
[KubernetesVersion.V1_25, "9.25.0"],
[KubernetesVersion.of("1.26"), "9.29.0"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lines 45 and 46 look duplicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i fully agree with you they look dublicate , but i tried to keep them as close as posssible before my commit. Check the lines in how they look into 1.8.1 with version for V1.25

Copy link
Collaborator

Choose a reason for hiding this comment

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

that is fine, as long as the map entry exists for the target version of EKS, it is the correct approach. Version may become different in the future.

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

LGTM

@shapirov103 shapirov103 merged commit c5ad957 into aws-quickstart:main Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants