Skip to content

Ignore 'v' version prefix in OCI artifact and Helm chart #990

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

Jump to bottom
Merged
merged 1 commit into from
May 27, 2024
Merged

Ignore 'v' version prefix in OCI artifact and Helm chart #990

merged 1 commit into from
May 27, 2024

Conversation

makkes
Copy link
Member

@makkes makkes commented May 14, 2024

Tools such as Bitnami's charts-syncer strip the v prefix from the chart version so that the OCI artifact version differs from the version defined in the chart's metadata. This leads to an error similar to this returned from h-c:

artifact revision 1.14.5 does not match chart version v1.14.5

This PR makes h-c ignore a leading v prefix in either the chart version of the OCI artifact tag.

@makkes makkes requested review from stefanprodan and souleb May 14, 2024 22:12
@stefanprodan
Copy link
Member

The v prefix is no valid for Helm charts, are you saying that you have a chart stored as vX.Y.Z in a Helm HTTP repo and charts-syncer removes it from Chart.yaml but not from the artifact tag?

@makkes
Copy link
Member Author

makkes commented May 15, 2024

The v prefix is no valid for Helm charts

I ran into this with cert-manager:

$ helm search repo jetstack/cert-manager                                                                                                                                                                                                                            
NAME                                    CHART VERSION   APP VERSION     DESCRIPTION                                                                                                                                                                                               
jetstack/cert-manager                   v1.14.5         v1.14.5         A Helm chart for cert-manager

They're using a v-prefixed version in their charts (that's also in the Chart.yaml).

are you saying that you have a chart stored as vX.Y.Z in a Helm HTTP repo and charts-syncer removes it from Chart.yaml but not from the artifact tag?

The other way around: charts-syncer stores the Helm artifact in OCI tagged as X.Y.Z but the Chart.yaml defines it as vX.Y.Z.

@stefanprodan
Copy link
Member

Ok, we can backport this to v1.0.x but I would wait a week or two before doing a patch release.

@stefanprodan
Copy link
Member

We should also allow the reverse situation, v prefix in the tag, but no v in Chart.yaml.

@makkes
Copy link
Member Author

makkes commented May 15, 2024

We should also allow the reverse situation, v prefix in the tag, but no v in Chart.yaml.

It catches both. I implemented a test for each case.

@makkes makkes added the backport:release/v1.0.x To be backported to release/v1.0.x label May 15, 2024
stefanprodan
stefanprodan approved these changes May 24, 2024
View reviewed changes
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 @makkes 🏅

@makkes
Ignore 'v' version prefix in OCI artifact and Helm chart
98ecb33 
Tools such as Bitnami's charts-syncer strip the `v` prefix from the
chart version so that the OCI artifact version differs from the
version defined in the chart's metadata. This leads to an error
similar to this returned from h-c:

```
artifact revision 1.14.5 does not match chart version v1.14.5
```

This commit makes h-c ignore a leading `v` prefix in either the chart
version of the OCI artifact tag.

Signed-off-by: Max Jonas Werner <max.werner@associmates.eu>
@makkes makkes merged commit ce58e47 into main May 27, 2024
@makkes makkes deleted the rev-vers branch May 27, 2024 06:58
@fluxcdbot
Copy link
Member

Successfully created backport PR for release/v1.0.x:

@fluxcdbot fluxcdbot mentioned this pull request May 27, 2024
Merged
@stefanprodan stefanprodan mentioned this pull request Aug 22, 2024
Closed
57 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanprodan stefanprodan stefanprodan approved these changes

@souleb souleb Awaiting requested review from souleb

Assignees
No one assigned
Labels
backport:release/v1.0.x To be backported to release/v1.0.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@makkes @stefanprodan @fluxcdbot