-
-
Notifications
You must be signed in to change notification settings - Fork 240
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
Conversation
a102703
to
5fdd864
Compare
I will fix these issues. |
Good idea. |
e24991c
to
b3bbc88
Compare
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). |
@felipecrs please fix issue. |
@yxxhero I fixed the GCI error but I can't identify what's wrong with the tests. |
@felipecrs please fix ci issue. Do you need help? |
@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. |
@yxxhero I think I fixed it now in a cleaner way. |
@felipecrs Please add some test. |
31f9e41
to
ece396e
Compare
@yxxhero I added one test, let me know if it's enough. |
fe0c4e1
to
5ce1e9b
Compare
d80084a
to
49acbfd
Compare
@felipecrs last step: please squash your commits. It's so much. |
Signed-off-by: Felipe Santos <felipecassiors@gmail.com>
0b778fc
to
a262db8
Compare
@yxxhero done. |
a262db8
to
f15bdbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -1,5 +1,7 @@ | |||
package helmexec | |||
|
|||
import "helm.sh/helm/v3/pkg/chart" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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.
Thank you too! :) |
Test: