-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 ParseTolerant to parse Helm version because of missing patch version #4712
Use ParseTolerant to parse Helm version because of missing patch version #4712
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #4712 +/- ##
=======================================
Coverage 73.38% 73.38%
=======================================
Files 341 341
Lines 13505 13505
=======================================
Hits 9910 9910
Misses 2976 2976
Partials 619 619
Continue to review full report at Codecov.
|
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.
Thanks for the PR @bdudelsack. Could you please add a test around this? We don't seem to have just a explicit test of binVer()
, but it seems like we should.
- add your v3.3 version to this set:
skaffold/pkg/skaffold/deploy/helm_test.go
Lines 346 to 353 in f7d1b6c
var ( // Output strings to emulate different versions of Helm version20rc = `Client: &version.Version{SemVer:"v2.0.0-rc.1", GitCommit:"92be174acf51e60a33287fb7011f4571eaa5cb98", GitTreeState:"clean"}\nError: cannot connect to Tiller\n` version21 = `Client: &version.Version{SemVer:"v2.15.1", GitCommit:"cf1de4f8ba70eded310918a8af3a96bfe8e7683b", GitTreeState:"clean"}\nServer: &version.Version{SemVer:"v2.16.1", GitCommit:"bbdfe5e7803a12bbdf97e94cd847859890cf4050", GitTreeState:"clean"}\n` version30b = `version.BuildInfo{Version:"v3.0.0-beta.3", GitCommit:"5cb923eecbe80d1ad76399aee234717c11931d9a", GitTreeState:"clean", GoVersion:"go1.12.9"}` version30 = `version.BuildInfo{Version:"v3.0.0", GitCommit:"e29ce2a54e96cd02ccfce88bee4f58bb6e2a28b6", GitTreeState:"clean", GoVersion:"go1.13.4"}` version31 = `version.BuildInfo{Version:"v3.1.1", GitCommit:"afe70585407b420d0097d07b21c47dc511525ac8", GitTreeState:"clean", GoVersion:"go1.13.8"}` ) - follow the example in other tests to override the
DefaultExecCommand
and usetestutil.CmdRunWithOutput()
Signed-off-by: Boris Dudelsack <bdudelsack@gmail.com>
f7d1b6c
to
bf016b2
Compare
Added the tests for helm v3.3. The official binaries from helm do output the semver compliant version number, but custom built do not. For example Manjaro has a custom buit helm version in their repository which outputs not semver compliant version number. |
When we can expect release with this fix? |
We're aiming to have a release on Wednesday. You can use the "bleeding edge" binaries — we typically cut our releases from HEAD. |
Fixes: #4711
Description
Use semVer ParseTolerant version to parse the Helm binary version because it does not follow all semver rules.