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

Support Helm 3.7.0 and new OCI Chart Format #1970

Merged
merged 1 commit into from
Oct 4, 2021
Merged

Conversation

aditmeno
Copy link
Contributor

@aditmeno aditmeno commented Sep 22, 2021

With the release of Helm 3.7.0, the OCI specific segments were completely reworked, causing helmfile to fail with OCI charts

This PR seeks to address the Helm specific changes required

HIP-006
Helm 3.7.0

NB -
Helm 3.7.0 has changed the way OCI charts are packaged, so they are no longer compatible with chats packaged with Helm 3.6.3 or earlier

You will need to repackage your OCI charts

@aditmeno aditmeno force-pushed the master branch 7 times, most recently from 7909986 to 7dbdb88 Compare September 22, 2021 16:30
@Laurianti
Copy link

Please, helm 3.6.3 doesn't support to install a 3.7.0 packaged helm chart:

Error: manifest does not contain a layer with mediatype application/tar+gzip]

@aditmeno aditmeno changed the title Support helm v3.7.0 Support Helm 3.7.0 Sep 24, 2021
Signed-off-by: Aditya Menon <[email protected]>
@dkirrane
Copy link

Looking for this too. What would be the oci format in helmfile. Is this correct?

releases:
- name: some-chart
  namespace: default
  chart: oci://myrepo.azurecr.io/charts/some-chart:0.1.1

@aditmeno
Copy link
Contributor Author

aditmeno commented Sep 24, 2021

Looking for this too. What would be the oci format in helmfile. Is this correct?

releases:
- name: some-chart
  namespace: default
  chart: oci://myrepo.azurecr.io/charts/some-chart:0.1.1

@dkirrane I've ensured oci:// get added in automatically to prevent disruptions to existing helmfiles using OCI charts

I think format below should work

releases:
- name: some-chart
  namespace: default
  chart: myrepo.azurecr.io/charts/some-chart
  version: 0.1.1

Though I'm open to changing it, where the user needs to explicitly specify oci://

Depends on what the other users prefer (explicit vs implicit oci://)

@mumoshu What would your opinion on this be?

@dkirrane
Copy link

Leaving out oci:// is good with me

@Laurianti
Copy link

I prefer removing the oci: true property, and explicitly adding the oci:// protocol

@aditmeno
Copy link
Contributor Author

aditmeno commented Sep 24, 2021

I prefer removing the oci: true property, and explicitly adding the oci:// protocol

The first part would be out of scope for this PR at least, since it'd require a lot of changes as to how helmfile differentiates a regular chart from an OCI one

The second part my only concern is, how many users/workflows would be affected since they'd need to edit their helmfile to accommodate this. Helm 3.7.0 was already an annoyance (giving no deprecation warning/timeline for the older OCI commands) which disrupted a lot of systems

@Laurianti
Copy link

You are right, and I agree with you, however there is to consider that the approach used by Helm for the management of the OCI was declared as experimental, and could be removed at any moment, to be used at your own risk.

In any case, in my opinion, it is possible to manage both approaches, to avoid breaking changes:

url: acr.azurecr.io
oci: true

and

url: oci://acr.azurecr.io

@aditmeno
Copy link
Contributor Author

You are right, and I agree with you, however there is to consider that the approach used by Helm for the management of the OCI was declared as experimental, and could be removed at any moment, to be used at your own risk.

In any case, in my opinion, it is possible to manage both approaches, to avoid breaking changes:

url: acr.azurecr.io
oci: true

and

url: oci://acr.azurecr.io

I'll try experimenting on this and see if I can get it to work

Keeping both options sounds like a good idea 👍

@eshepelyuk
Copy link

Hello
Do you have any estimation on releasing a new version with this fix ?

@sbckr
Copy link

sbckr commented Oct 1, 2021

+1
Urgently waiting for the compatibility update

@whyman
Copy link

whyman commented Oct 1, 2021

@roboll Are there any blockers we need to resolve to get have this merged?

@aditmeno aditmeno changed the title Support Helm 3.7.0 Support Helm 3.7.0 and Support new OCI Chart Format Oct 1, 2021
@aditmeno
Copy link
Contributor Author

aditmeno commented Oct 1, 2021

@mumoshu Could I get a review for my PR?

@aditmeno aditmeno changed the title Support Helm 3.7.0 and Support new OCI Chart Format Support Helm 3.7.0 and new OCI Chart Format Oct 3, 2021
Copy link
Collaborator

@mumoshu mumoshu 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 for the enhancement @aditmeno!

@mumoshu
Copy link
Collaborator

mumoshu commented Oct 4, 2021

@eshepelyuk @sbckr @whyman I don't usually say things like this as I believe users know what they're doing but anyway- You shouldn't use any experimental features in a production-like environment that requires urgency in enhancements like this!

Ah..., or maybe we lack documentation on the helmfile side to say that helm's OCI support is experimental?
I had been blindly assuming everyone reads helm doc before using helmfile so they might know helm's OCI support is experimental, but I'm now doubting if the expectation is correct.

I can get important feedbacks like this only when it breaks so I'd appreciate your feedback.

@mumoshu mumoshu merged commit 9a0ce53 into roboll:master Oct 4, 2021
@sbckr
Copy link

sbckr commented Oct 4, 2021

@mumoshu I really like the OCI support in helm and helmfile, and we use this feature in our alpha-ish / development environment. The recent changes to helm broke our convenience setup, and there would have been many ways to work around the issue that arose, but I wanted to express my support for the enhancement provided by @aditmeno and OCI support in general.

Regarding your question, helm indicates in its documentation or feature abstract that the support for OCI is experimental. Since helmfile adds on top of helm, it is more or less implicit that this feature is experimental, but unless I am missing something elsewhere, it is not written in helmfile's documentation directly. If you want to make it explicit, I'd suggest to add a note to oci-registries referring to Helm - Registries or OCI Feature Abstract.

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.

7 participants