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: fix for comparing semver for enterprise versions #857

Merged
merged 2 commits into from
Aug 14, 2023
Merged

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Aug 10, 2023

What this PR does / why we need it:

This fixes an issue where an enterprise version (like e.g. 3.4.0.0) wouldn't be allowed to be provided.

Which issue this PR fixes

Fixes #856

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • New or modified sections of values.yaml are documented in the README.md
  • Commits follow the Kong commit message guidelines

@pmalek pmalek self-assigned this Aug 10, 2023
@pmalek pmalek force-pushed the fix-semver branch 7 times, most recently from 1edcbc7 to 2675d75 Compare August 10, 2023 19:12
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

My kingdom for Enterprise using standard semver build tags exclusively for its extra stuff so it at least parses and sticks its mystery values in the designated mystery value box.

What's the intended effect of the change? Regex inscrutability on top of the normal template mess inscrutability will be hard to read in the future, so we should have an explanatory comment inline explaining how this intends to parse input.

We presumably need ^ anchors, correct? What happens if we get a 4.3.4.1 where the tail is a valid semver version we'd compare against?

@randmonkey
Copy link
Contributor

randmonkey commented Aug 11, 2023

What if the latest tag is used (as used in KIC nightly integration tests)?

@pmalek pmalek marked this pull request as ready for review August 11, 2023 10:46
@pmalek pmalek requested a review from a team as a code owner August 11, 2023 10:46
@pmalek
Copy link
Member Author

pmalek commented Aug 11, 2023

We presumably need ^ anchors, correct? What happens if we get a 4.3.4.1 where the tail is a valid semver version we'd compare against?

Yes, added that.

I've simplified the change to just extract the first 3 segments if those are detected, otherwise fallback to old behaviour. This will fix the 4 segment versions and leave the old behaviour otherwise.

@pmalek pmalek requested a review from rainest August 11, 2023 10:47
@pmalek
Copy link
Member Author

pmalek commented Aug 11, 2023

Not sure why the tests are failing 😠 https://github.com/Kong/charts/actions/runs/5831706278/job/15815656070?pr=857#step:10:2018 the pods deploy successfully 🤔

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Problem diagnosis: chart-testing is terrible at reporting issues. --helm-extra-args "--debug" will make Helm explain its reasoning, because for some reason it doesn't actually explain them on failure.

Do not use chart-testing's --debug in most cases, since it suppresses Helm debug output for some reason.

Deleting the other CI files temporarily makes the output less of a mess.

Problem cause: chart-testing's KIND does not use any LB provider, and Helm will block installs if a spawned LB never gets addresses. All the CI entries need proxy.type: NodePort.

@rainest rainest enabled auto-merge (squash) August 14, 2023 20:27
@rainest rainest merged commit d482a55 into main Aug 14, 2023
17 checks passed
@rainest rainest deleted the fix-semver branch August 14, 2023 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chart Image Not Valid Semantic Version
3 participants