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

Replace _ with + when verifying the chart version matches the OCI artifact tag #1102

Merged

Conversation

baburciu
Copy link
Contributor

Closes #1099

OCI tags are not semver compatible, with + character not allowed. Helm worked around this by converting + to _ before pushing to the OCI repo and vice versa when pulling from the repo.

This PR makes helm-controller adhere to that convention, by converting OCI artifact tag using _ to + replacement prior to any check or processing.

@baburciu baburciu force-pushed the accept-underscore-ocirepo-tag-hr branch 2 times, most recently from adcbcaf to b2db396 Compare October 31, 2024 16:23
@baburciu
Copy link
Contributor Author

I'm not sure what should be best returned as error

return "", fmt.Errorf("failed parsing artifact revision %s", tagD[0])

or

return "", fmt.Errorf("failed parsing artifact revision %s", tagConverted)

but the conversion needs to happen prior to calling semver.NewVersion

@stefanprodan
Copy link
Member

I think we should return the one with a + as that's the string we try to validate as semver.

PS. This PR needs a test using a tag that has _.

@baburciu baburciu marked this pull request as draft November 1, 2024 08:38
@baburciu baburciu force-pushed the accept-underscore-ocirepo-tag-hr branch from b2db396 to 08cb29c Compare November 1, 2024 12:20
Signed-off-by: Bogdan-Adrian Burciu <bogdanadrian.burciu@yahoo.com>
@baburciu baburciu force-pushed the accept-underscore-ocirepo-tag-hr branch from 08cb29c to caf49d2 Compare November 1, 2024 12:42
@baburciu
Copy link
Contributor Author

baburciu commented Nov 1, 2024

thank you, I've added a test for it, which is passing make test. I'm new to this, so if it's not that good, please inform

@baburciu baburciu marked this pull request as ready for review November 1, 2024 12:46
Copy link
Member

@stefanprodan stefanprodan 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 @baburciu 🥇

@stefanprodan stefanprodan added the area/oci OCI related issues and pull requests label Nov 1, 2024
@stefanprodan stefanprodan changed the title Replace _ with + for OCI artifacts tags when pulled for helm Replace _ with + when verifying the chart version matches the OCI artifact tag Nov 1, 2024
@stefanprodan stefanprodan merged commit 5beaf80 into fluxcd:main Nov 1, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oci OCI related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OCI artifact semver alignment with Helm for tags with underscore
2 participants