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

feat: add support for helm compatible binaries like werf #211

Merged
merged 4 commits into from
Oct 10, 2022

Conversation

himanshu-garg-razorpay
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
API breaks? no
Deprecations? no
Related tickets fixes #210
License Apache 2.0

What's in this PR?

Removes the code checking for the "validity" of the helm binary provided.
It was a brittle mechanism grepping for the current documentation on calling help.
This breaks not just for different helm compatible binaries like werf but also if helm itself changes its documentation.
The "validity" of a binary is hard to define in a way that allows interop with all future helm compatible binaries.
Also, I don't think we'll introduce any bugs by not checking this because when someone passes an "invalid" binary it will just not work. On the other hand, if it works, who are we to say it's not a valid binary. If it walks like a duck, quacks like a duck, it is a duck.

Checklist

  • Code meets the Developer Guide
  • User guide and development docs updated (if needed)
  • Related Helm chart(s) updated (if needed)

@himanshu-garg-razorpay himanshu-garg-razorpay changed the title Adds support for other helm compatible binaries like werf Adds support for helm compatible binaries like werf Aug 23, 2022
@aslafy-z
Copy link
Owner

aslafy-z commented Sep 23, 2022

Thank you for the contribution. I think you'll have to change all references from "$HELM_BIN" to $HELM_BIN, i.e. Remove the quotes for it to work with HELM_BIN="werf helm".
Can you add a test with a mocking script like:

#!/bin/sh
exec $@

and call it with HELM_BIN="/path/to/script.sh helm" in the test to ensure helm-git keeps support for multi word 'executables'

@himanshu-garg-razorpay
Copy link
Contributor Author

@aslafy-z for werf, you don't use HELM_BIN="werf helm", instead you use HELM_BIN="werf" (with some environment variable asking werf to maintain helm compatibility) and for any other helm compatible binary, single worded usage should be enough as anything multi-worded can be trivially converted to a single word option (using a script)

And yes I understand that there might be a use-case for multi-worded executables especially to not have to write a script, however, that's not really what I'm trying to target here, primarily because it increases my test surface (what about HELM_BIN='do_x && helm') as you rightly pointed out.

I'll be happy to create a separate issue for multi word support in HELM_BIN to look for use-cases in the community.

@aslafy-z aslafy-z changed the title Adds support for helm compatible binaries like werf feat: add support for helm compatible binaries like werf Oct 10, 2022
@aslafy-z
Copy link
Owner

@himanshu-garg-razorpay lgtm! Can you please rebase your PR before I merge it? Thank you

@himanshu-garg-razorpay
Copy link
Contributor Author

himanshu-garg-razorpay commented Oct 10, 2022

@himanshu-garg-razorpay lgtm! Can you please rebase your PR before I merge it? Thank you

@aslafy-z Done. Please go ahead.

@himanshu-garg-razorpay
Copy link
Contributor Author

himanshu-garg-razorpay commented Oct 10, 2022

@aslafy-z thanks for the approval and running the workflows. The workflow checks have passed. Shall we merge this now..?

@aslafy-z aslafy-z merged commit 43905d5 into aslafy-z:master Oct 10, 2022
@himanshu-garg-razorpay
Copy link
Contributor Author

@aslafy-z if there's nothing else already planned to be released (not seeing any open PRs), please create a new release so that we can start deprecating our internal fork

@aslafy-z
Copy link
Owner

🎉 This PR is included in version 0.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aslafy-z aslafy-z added the released This issue/pull request has been released. label Oct 10, 2022
@aslafy-z
Copy link
Owner

@himanshu-garg-razorpay the release pipeline was stuck for some reason. It should be good by now 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm-git not working with werf:
2 participants