-
Notifications
You must be signed in to change notification settings - Fork 205
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
[Karpenter] Fix CRDs URL not found #1031
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vumdao the version is expected to be set in the format v${semver}
. Please see the current setting here.
Here is what we set in the test: https://github.com/aws-quickstart/cdk-eks-blueprints/blob/main/examples/blueprint-construct/index.ts#L152, also all our docs have the same convention.
If your fix is applied, seems like it will break the current logic, as the CRD url will have double v: vv0.34.0
.
The docs may not articulate that aspect enough, that may something we should fix.
Please let me know your thoughts, and decide how to move forward with this.
Hi @shapirov103 , thanks for your review, I updated the code to align with the various versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vumdao please see my remaining comment
lib/addons/karpenter/index.ts
Outdated
// Since v0.35.0, Karpenter OCI tags and Helm chart version are now valid semantic versions, | ||
// meaning that the v prefix from the git tag has been removed and they now follow the x.y.z pattern | ||
// But the CRDs URL still uses the git tag format, so we need to check for the version and adjust the URL accordingly | ||
let _version = (semver.gte(version, "0.35.0")) ? `v${version}` : version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vumdao alsmost there.
I looked through the branches of Karpenter and it does not look like the version convention with v
is something specifically enforced before 0.35.0.
Let's add an explicit check: if(!version.startsWith('v') then add
v` into the version in URL, to avoid potential mismatches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you
Issue
If
installCRDs
enabled, it got following errorDescription of changes:
Change
${version}
tov${version}
eg. https://raw.githubusercontent.com/aws/karpenter/v0.37.0/pkg/apis/crds/karpenter.sh_nodepools.yaml
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.