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

feat: use helmBin() for kustomize build for helmChart in kustomization.yaml #64

Merged
merged 3 commits into from
Nov 12, 2023
Merged

feat: use helmBin() for kustomize build for helmChart in kustomization.yaml #64

merged 3 commits into from
Nov 12, 2023

Conversation

moririnson
Copy link
Contributor

@moririnson moririnson commented Nov 6, 2023 •
edited
Loading

This adds support --helm-binary for helmChart in kustomization.yaml by:

  • Changing use helmBin() for kustomize build

Resolves helmfile/helmfile#1121

@moririnson moririnson changed the title feat: do whatever for whatever purpose feat: use helmBin() for kustomize build for helmChart in kustomization.yaml Nov 6, 2023
@moririnson moririnson force-pushed the kustomize-respect-helm-binary branch 2 times, most recently from 2130c9a to 108cd55 Compare November 6, 2023 13:37
@moririnson moririnson mentioned this pull request Nov 6, 2023
Closed
@yxxhero
Copy link
Member

yxxhero commented Nov 10, 2023

@moririnson could you add some tests?

@moririnson moririnson force-pushed the kustomize-respect-helm-binary branch 3 times, most recently from 68c99ff to 9b075f6 Compare November 10, 2023 19:38
@moririnson
feat: use helmBin() for kustomize build for helmChart in kustomizatio…
1ef6584 
…n.yaml

This adds support --helm-binary for helmChart in kustomization.yaml by:

- Changing use helmBin() for kustomize build

Resolves helmfile/helmfile#1121

Signed-off-by: moririnson <[email protected]>
@moririnson
Copy link
Contributor Author

@yxxhero I added tests. Please review 🙇

In the first commit, I added cases for --enable-alpha-plugins=true (current helmfile) and --enable-alpha-plugins=false . The --enable-alpha-plugins=false case is expected to fail.

In the next commit, as the title of the pull request suggests, I made it so that helmBin() is also carried over to kustomize build. Both test cases for --enable-alpha-plugins=true/false should succeed.

Interestingly, it seems that --enable-helm is enabled when --enable-alpha-plugins=true. I couldn't find any description to that effect, so I might be misunderstanding something.

yxxhero
yxxhero reviewed Nov 11, 2023
View reviewed changes
kustomize.go Outdated Show resolved Hide resolved
@moririnson moririnson force-pushed the kustomize-respect-helm-binary branch 2 times, most recently from a08d70f to f72a52c Compare November 11, 2023 02:08
yxxhero
yxxhero approved these changes Nov 12, 2023
View reviewed changes
Copy link
Member

@yxxhero yxxhero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
tests added.

@yxxhero yxxhero merged commit 7da5efd into helmfile:master Nov 12, 2023
3 checks passed
@moririnson moririnson deleted the kustomize-respect-helm-binary branch November 13, 2023 01:14
@moririnson
Copy link
Contributor Author

moririnson commented Nov 13, 2023 •
edited
Loading

@yxxhero Should I submit a p-r to use chartify 0.17.0 on the helmfile side? Or should I wait for dependabot?
I hope I can use this fixes in my work.Thank you.

@yxxhero
Copy link
Member

yxxhero commented Nov 13, 2023

@moririnson you can post a PR. thanks so much.

@yxxhero
Copy link
Member

yxxhero commented Nov 13, 2023

@moririnson you can post a PR. thanks so much.

I will merge it as soon as I can.

@moririnson moririnson mentioned this pull request Nov 13, 2023
Merged
@moririnson
Copy link
Contributor Author

@yxxhero I posted PR. Please merge when you can~ 🙏
https://github.com/helmfile/helmfile/pull/1140/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yxxhero yxxhero yxxhero 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.

Helmfile doesn't respect --helm-binary option when using helmCharts in kustomization.yaml
2 participants
@moririnson @yxxhero