nflaig / semantic-release-helm

semantic-release plugin to publish Helm charts
MIT License
13 stars 11 forks source link

fix: remove oci:// prefix from registry host when login #14

Closed alita1991 closed 1 year ago

alita1991 commented 1 year ago

Closes #8

nflaig commented 1 year ago

Do you have a reference on why this is all of the sudden needed with helm version > 3.10?

This solution feels a bit hacky to me. Based on the helm version the registry needs to be defined in a different format with or without oci:// prefix. Maybe it would be better to handle this internally so there is no difference on how the registry URI needs to be set. See https://github.com/nflaig/semantic-release-helm/issues/8#issuecomment-1489241000

Maybe @lsoica can take a look as well and give feedback

alita1991 commented 1 year ago

Hi @nflaig

You can read about this at https://helm.sh/docs/topics/registries/

This issue is also similar to mine https://github.com/helm/helm/issues/10814

This change is only required before we log in to the registry. After we log in, we will use the registry defined in the config.

nflaig commented 1 year ago

thanks for the links, looks like helm just doesn't care about following semver (actually oci was experimental so I guess breaking changes are fine in this case)

I really think this should be implemented differently by updating the push command else once users upgrade to helm versions >= 3.8.0 their setup will break because they will have their registry URI set without oci:// prefix.

image

alita1991 commented 1 year ago

@nflaig the change is only required when we login to the registry, after this, we just have to use the registry defined in the config. Do you think the change I made will impact also other commands like push?

nflaig commented 1 year ago

My main concern is just that once others users upgrade to helm version 3.8.0 their setup will break because they did not explicity specify oci:// in their registry URI

alita1991 commented 1 year ago

Make sense, give me some time to update the code, will let you know

nflaig commented 1 year ago

Do you by any chance know if helm push breaks in version 3.7.0 if you specify the oci:// prefix? If that is not the case we can just always add it on push (if not present), and just for the sake of it always remove it on login

alita1991 commented 1 year ago

Hi @nflaig , I found this post https://blog.devgenius.io/helm-3-7-changes-to-oci-registry-16f2887e0704 I believe this answers your question, it will not break in 3.7.0.

Please check the latest changes and let me know what you think.

nflaig commented 1 year ago

I found this post https://blog.devgenius.io/helm-3-7-changes-to-oci-registry-16f2887e0704 I believe this answers your question, it will not break in 3.7.0.

great thanks for checking, this makes it a bit simpler to implement

alita1991 commented 1 year ago

If I modify the pluginConfig.registry in the verifyConditions.js default function, will remain modified in the other scripts as well? like publish.js?

alita1991 commented 1 year ago

@nflaig please check, I made more changes

nflaig commented 1 year ago

If I modify the pluginConfig.registry in the verifyConditions.js default function, will remain modified in the other scripts as well? like publish.js?

No, strings are not modifiable like this, you always need to reassign, else the .split would be an issue already

alita1991 commented 1 year ago

@nflaig I believe it should be fine now

nflaig commented 1 year ago

Published semantic-release-helm3@2.4.2