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.

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

fix: correct helm binary in install script for environment that have both helm 2 and 3 installed #270

Merged
merged 1 commit into from
May 18, 2021
Merged

fix: correct helm binary in install script for environment that have both helm 2 and 3 installed #270

merged 1 commit into from
May 18, 2021

Conversation

astorath
Copy link
Contributor

This fixes install-binary.sh to use correct helm binary

@deiga
Copy link
Contributor

deiga commented Mar 21, 2021

@astorath Can you describe what the problem is right now? Why is this fix needed?

@astorath
Copy link
Contributor Author

@deiga Sure, the problems are, when you have helm binary named differently (helm2 or helm3 for instance):

  1. Installation does not work sometimes (HELM_PLUGIN_DIR resolves to /plugins/helm-diff) because root fs might be readonly
  2. /plugins/helm-diff is created polluting filesystem
  3. The latest version is installed always (because git describe fails on empty /plugins/helm-diff)

PS: I also have a question about that. Would you be so kind to release special v2.11.0+6 tagged v2 release containing the same fix. I have github fork for that, but it would be nice to have the same issue fixed for helm2 in original repo.

@mumoshu mumoshu changed the title fix: correct helm binary in install script fix: correct helm binary in install script for environment that have both helm 2 and 3 installed May 18, 2021
mumoshu
mumoshu approved these changes May 18, 2021
View reviewed changes
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.

Good catch! I've tried my best to rephrase the PR title to better capture the purpose of this fix. Thanks for your contribution @astorath ☺️

@mumoshu mumoshu merged commit 7f10f25 into databus23:master May 18, 2021
@mumoshu
Copy link
Collaborator

mumoshu commented May 18, 2021

@deiga Thanks for your help on communication! I wouldn't have understood this without your conversation with @astorath

@mumoshu mumoshu mentioned this pull request Jun 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mumoshu mumoshu mumoshu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@astorath @deiga @mumoshu