Skip to content

Support Helm semver encoding in OCI repositories #1834

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.

Sign up for GitHub

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

Open
evankanderson wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Support Helm semver encoding in OCI repositories #1834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
314c103
Support Helm semver encoding in OCI repositories
evankanderson Jun 16, 2025
f772b6e
Address reviewer feedback; only trigger for Helm charts
evankanderson Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions internal/controller/ocirepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,7 +898,11 @@ func (r *OCIRepositoryReconciler) getTagBySemver(repo name.Repository, exp strin

var matchingVersions []*semver.Version
for _, t := range validTags {
v, err := version.ParseVersion(t)
// Helm translates `+` to `_` in OCI tags, because `+` is not a valid tag character.
versionStr := strings.Replace(t, "_", "+", 1)
// It would be even better to use `org.opencontainers.image.version` annotation
// if present, but that adds a fetch for each tag.
Comment on lines +905 to +906
Copy link
Copy Markdown
Member

@stefanprodan stefanprodan Apr 30, 2026

Choose a reason for hiding this comment

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

Delete this comment please, we never considered fetching artifacts to determine the version.

v, err := version.ParseVersion(versionStr)
Copy link
Copy Markdown
Member

@stefanprodan stefanprodan Apr 29, 2026

Choose a reason for hiding this comment

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

OCIRepository is not specific to Helm in any way, we are going to break versions that contain _ as suffix. For this replacement to be safe, we should only do it if the layer selector is set to the Helm OCI type https://fluxcd.io/flux/components/helm/helmreleases/#ocirepository-reference-example

Copy link
Copy Markdown
Author

@evankanderson evankanderson Apr 29, 2026

Choose a reason for hiding this comment

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

I'm willing to do that; another option is to do this only if ParseVersion produces an error. It looks to me like both semver.StrictNewVersion and semver.NewVersion both reject (return err for) version numbers that contain _ at all, so our risk would be that we would accept versions that we currently reject.

Copy link
Copy Markdown
Author

@evankanderson evankanderson Apr 29, 2026

Choose a reason for hiding this comment

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

Interestingly, in the tests I didn't need to actually set the media type on the podinfo images for the test to pass.

if err != nil {
continue
}
Expand All @@ -913,7 +917,8 @@ func (r *OCIRepositoryReconciler) getTagBySemver(repo name.Repository, exp strin
}

sort.Sort(sort.Reverse(semver.Collection(matchingVersions)))
return repo.Tag(matchingVersions[0].Original()), nil
asTag := strings.Replace(matchingVersions[0].Original(), "+", "_", 1)
return repo.Tag(asTag), nil
}

// keychain generates the credential keychain based on the resource
Expand Down
5 changes: 3 additions & 2 deletions internal/controller/ocirepository_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2866,6 +2866,7 @@ func TestOCIRepository_getArtifactRef(t *testing.T) {
"6.1.5",
"6.1.6-rc.1",
"6.1.6",
"6.1.7_ref.1234567", // Version 6.1.7+ref.1234567, encoded as a tag
)
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -2898,12 +2899,12 @@ func TestOCIRepository_getArtifactRef(t *testing.T) {
want: "ghcr.io/stefanprodan/charts@" + imgs["6.1.6"].digest.String(),
},
{
name: "valid url with semver reference",
name: "valid url with semver reference and build identifier",
Copy link
Copy Markdown
Member

@stefanprodan stefanprodan Apr 30, 2026

Choose a reason for hiding this comment

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

Please add a new test case for the build identifier. For the existing semver test case, set semver: 6.1.x, for the new one set semver: 6.2.x. Also rename the tar to podinfo-6.2.1_ref.9f4969c.tar.

Copy link
Copy Markdown
Member

@stefanprodan stefanprodan Apr 30, 2026

Choose a reason for hiding this comment

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

We also need a test to prove that ordering will be random and not what people expect, because Git commit SHAs are not numeric. Having the same version and two build tags, will not give you the "latest" one.

url: fmt.Sprintf("oci://%s/podinfo", server.registryHost),
reference: &sourcev1.OCIRepositoryRef{
SemVer: ">= 6.1.6",
},
want: server.registryHost + "/podinfo:6.1.6",
want: server.registryHost + "/podinfo:6.1.7_ref.1234567",
},
{
name: "invalid url without oci prefix",
Expand Down
Binary file added internal/controller/testdata/podinfo/podinfo-6.1.7_ref.1234567.tar
View file
Open in desktop
Binary file not shown.