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

Use helm show chart to identify chart version #395

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Sep 27, 2022

Test:

$ helm fetch https://github.com/jenkinsci/helm-charts/releases/download/jenkins-4.2.6/jenkins-4.2.6.tgz
$ mv -f jenkins-4.2.6.tgz jenkins.tgz

$ helmfile sync
UPDATED RELEASES:
NAME      CHART           VERSION
jenkins   ./jenkins.tgz          

$ ./helmfile sync
UPDATED RELEASES:
NAME      CHART           VERSION
jenkins   ./jenkins.tgz     4.2.6

@felipecrs
Copy link
Contributor Author

I will fix these issues.

@yxxhero
Copy link
Member

yxxhero commented Sep 27, 2022

Good idea.

@felipecrs felipecrs force-pushed the helm-show-version branch 2 times, most recently from e24991c to b3bbc88 Compare September 27, 2022 23:44
@felipecrs
Copy link
Contributor Author

Ok, both lint and tests should pass now. I'm not sure about the approach I used to avoid tests from failing, Golang isn't my best skill. So, if you think it's easier/faster to just fixup the code by yourself, feel totally free (otherwise it's totally ok to provide comments as well).

@yxxhero
Copy link
Member

yxxhero commented Sep 28, 2022

@felipecrs please fix issue.

@felipecrs
Copy link
Contributor Author

@yxxhero I fixed the GCI error but I can't identify what's wrong with the tests.

@yxxhero
Copy link
Member

yxxhero commented Sep 28, 2022

@felipecrs please fix ci issue. Do you need help?

@yxxhero
Copy link
Member

yxxhero commented Sep 28, 2022

@felipecrs from origin code. It can't get chart version. but your code can do it. so you should change the test case log output.

pkg/state/state.go Outdated Show resolved Hide resolved
@felipecrs
Copy link
Contributor Author

@yxxhero I think I fixed it now in a cleaner way.

@yxxhero
Copy link
Member

yxxhero commented Sep 29, 2022

@felipecrs Please add some test.

@felipecrs
Copy link
Contributor Author

@yxxhero I added one test, let me know if it's enough.

pkg/app/mocks_test.go Outdated Show resolved Hide resolved
pkg/app/app_test.go Outdated Show resolved Hide resolved
pkg/exectest/helm.go Outdated Show resolved Hide resolved
@yxxhero
Copy link
Member

yxxhero commented Oct 4, 2022

@felipecrs last step: please squash your commits. It's so much.

Signed-off-by: Felipe Santos <felipecassiors@gmail.com>
@felipecrs
Copy link
Contributor Author

@yxxhero done.

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

@yxxhero
Copy link
Member

yxxhero commented Oct 4, 2022

@mumoshu

@@ -1,5 +1,7 @@
package helmexec

import "helm.sh/helm/v3/pkg/chart"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is our first attempt to use Go helm pkg directly rather than calling out to the helm command.
I don't know if it works out in the long term but it would be worth trying.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yxxhero If this is okay, we might be able to replace every invocation to helm command with the respective call to helm.sh/helm/v3/pkg/whatever structs and functions. That would free our users from being required to make Helmfile and Helm versions in-sync. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to not relying on the help binary, which should require many changes and can be optimized in the future. @mumoshu

Copy link
Contributor

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

LGTM. Thanks a lot for your efforts! This is awesome.

@yxxhero yxxhero merged commit e92c71f into helmfile:main Oct 5, 2022
@felipecrs felipecrs deleted the helm-show-version branch October 5, 2022 02:10
@felipecrs
Copy link
Contributor Author

Thank you too! :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants