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

fix: rename push cmd to work with new helm version #110

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

emanuelflp
Copy link
Contributor

As helm 3.7.x turn helm push a top level command, we can't use it more to this plugin.

To continue this plugin working, we just need to rename the command that will be mapped by helm.

@sathieu
Copy link
Contributor

sathieu commented Sep 22, 2021

@emanuelflp Maybe rename the command upload or http-push ? This is not really legacy, as HTTP repositories are still standard.

@sathieu
Copy link
Contributor

sathieu commented Sep 22, 2021

@jdolitsky Please review this MR... helm-push is currently broken with latest Helm v3.7.0 (see #109).

@maxisam
Copy link

maxisam commented Sep 22, 2021

This is awesome! Thank you! on naming, do you think cm-push might fit to its meaning more? That said, naming is the hardest problem in computer science lol

@c0dearm
Copy link

c0dearm commented Sep 22, 2021

I vote for cm-push too

@sathieu
Copy link
Contributor

sathieu commented Sep 22, 2021

What about http-push or upload ? This command can be used with Gitlab Helm's repo too, this would help to use a more common name here...

@jdolitsky What about merging helm push plugin in Helm itself?

@jdolitsky
Copy link
Contributor

Hey everyone - sorry for backlog on this project

The ultimate solution here is to add support in helm/helm for "Uploader Plugins", similar to "Downloader Plugins": https://helm.sh/docs/topics/plugins/#downloader-plugins

This would allow us to natively push to ChartMuseum by defining a protocol (such as cm://).

If somebody wants to take this forward, I had some working code here: https://github.com/bloodorangeio/helm/blob/hip-6-push/pkg/pusher/pluginpusher.go

This was originally part of helm/helm#9782, but removed in order to shrink the size of change and to get it merged.

This would result in this plugin having an uploaders section in plugin.yaml such as this:

uploaders:
- command: "bin/helmpush"
  protocols:
  - "cm"

This plugin is actually already technically a valid "Downloader" plugin: https://github.com/chartmuseum/helm-push#custom-downloader

Once all this is in place, we would be able to do something like this nativiely from Helm:

helm push mychart-1.2.3.tgz cm://my.chart.repo.com

@jdolitsky
Copy link
Contributor

I've opened helm/helm#10175 to capture the comment above

@jdolitsky
Copy link
Contributor

I think regardless of whether this is implemented, the name push will still conflict. I too vote for cm-push

plugin.yaml Outdated Show resolved Hide resolved
@eytanhanig
Copy link

Some of us are still using Helm 3.6.* due to bugs in Helm 3.7.0. If we do change the plugin name/helm command please ensure that there is a major increment in the plugin version to signal the introduction of a breaking change.

plugin.yaml Show resolved Hide resolved
@scbizu
Copy link
Contributor

scbizu commented Sep 28, 2021

@jdolitsky Could we possible default helm push to use helm internal ONLY IF users open up the experiment flag with export HELM_EXPERIMENTAL_OCI=1 , otherwise , just keep their local plugin ?

HELM_EXPERIMENTAL_OCI=1 helm push xxxx  // helm internal push with OCI
helm push xxx // local plugin with non-OCI like CM

@bacongobbler
Copy link

bacongobbler commented Sep 29, 2021

please ensure that there is a major increment in the plugin version to signal the introduction of a breaking change.

Generally speaking this is a non-issue for software that have not yet reached v1.0.0. The helm-push plugin is at v0.9.0. Bumping the major version would mean a v1.0.0 release... Not ideal for the authors because v1.0.0 indicates to most people that this is now stable. When in reality not much has changed - only the name.

Bumping to v0.10.0 with a notice that a breaking change has been introduced is more appropriate in this case.

@bacongobbler
Copy link

bacongobbler commented Sep 29, 2021

Could we possible default helm push to use helm internal ONLY IF users open up the experiment flag with export HELM_EXPERIMENTAL_OCI=1 , otherwise , just keep their local plugin ?

Hi. Helm core maintainer here.

I would be concerned about the security implications of that proposal.

Imagine you implemented this in Helm 3.8, and helm push was marked as GA in Helm 3.9. An attacker could ask someone to helm-3.9 plugin install their plugin, which would be masked by anyone using helm-3.9 push. But if the attacker can get the user to run helm-3.8 push, their plugin would execute and the user would have no idea. And if hypothetically we removed or re-named helm push in Helm 4, the same security concerns would arise there as well.

That is why Helm returns an error when a plugin collides with a builtin command.

In any case, a change like that would have to be proposed to Helm's core. But security issues related to masking malicious attacks come front-of-mind here and I'm not sure there's a way to work around that.

@scbizu
Copy link
Contributor

scbizu commented Sep 30, 2021

Imagine you implemented this in Helm 3.8, and helm push was marked as GA in Helm 3.9. An attacker could ask someone to helm-3.9 plugin install their plugin, which would be masked by anyone using helm-3.9 push. But if the attacker can get the user to run helm-3.8 push, their plugin would execute and the user would have no idea. And if hypothetically we removed or re-named helm push in Helm 4, the same security concerns would arise there as well.

Good point . And I think again about my proposal , I found it would tie helm-push with helm tightly. To implement what I proposed , we should invoke helm-push plugin inside helm push which is not appropriate .

Under this case , I think helm-push may should add the other alias for helm push, such as helm cm-push. And for those people who:

  • Bump Helm to 3.7.x , should update helm-push to use cm-push for cm repo , and use push for OCI repo (@emanuelflp).
  • Bump helm-push but not bump Helm to 3.7.x , cm-push and push both works . (@eytanhanig ) .

After this update , we will mark push command as deprecated and removed once OCI repo is stable.

Should we pin this issue up to make it more attractive ?

Any Thoughts or suggestions ? /cc @jdolitsky @bacongobbler

And I can self-assign this issue XD

@ysma500
Copy link

ysma500 commented Sep 30, 2021

Hi helm-push community,
I just found the source of my Github Action workflow fail by coming here. I must say that a resolution on this thread is important as the base Github Action Ubuntu 20.04 use Helm v3.7.0 as seen here. I second the use of helm cm-push command 👍 . Love the product, been using it for a year now.

Signed-off-by: Josh Dolitsky <[email protected]>
@jdolitsky jdolitsky merged commit d7cc009 into chartmuseum:master Oct 4, 2021
@ysma500
Copy link

ysma500 commented Oct 4, 2021

@jdolitsky Do we need to wait for a new release tag to use this new feature?

@bacongobbler
Copy link

v0.10.0 was released 30 minutes ago. https://github.com/chartmuseum/helm-push/releases/tag/v0.10.0

robertlemke added a commit to flownative/docker-action-helm-release that referenced this pull request Oct 11, 2021
"helm push" is not available anymore, it was renamed to "helm cm-push".
See chartmuseum/helm-push#110
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.

10 participants