-
Notifications
You must be signed in to change notification settings - Fork 705
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
Fix get to retrieve the latest release #708
Conversation
pkg/proxy/proxy.go
Outdated
break | ||
} | ||
r := l[0] | ||
if (namespace == "" || namespace == r.Namespace) && r.Name == name { |
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.
Why do we need to check for the namespace and name again here, when we already are filtering it in ListReleases?
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.
true, it's not needed anymore
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.
the problem is that some unit tests rely in that logic (to double filter the results) but that should not be necessary.
@@ -347,7 +346,7 @@ func TestDeleteMissingHelmRelease(t *testing.T) { | |||
app := AppOverview{"foo", "1.0.0", "my_ns", "icon.png", "DEPLOYED"} | |||
proxy := newFakeProxy([]AppOverview{app}) | |||
|
|||
err := proxy.DeleteRelease(app.ReleaseName, "other_ns", true) | |||
err := proxy.DeleteRelease("not_foo", "other_ns", true) |
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.
shouldn't this be rs2?
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.
not in this test (those var are not defined here)
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.
ah sorry, GitHub was hiding some lines - my bad!
proxy := newFakeProxy([]AppOverview{app}) | ||
|
||
_, err := proxy.UpdateRelease(rs, ns, "", ch) | ||
if err == nil { | ||
t.Error("Update should fail, there is not a release in the namespace specified") | ||
} | ||
if !strings.Contains(err.Error(), "not found") { | ||
if !strings.Contains(err.Error(), "No such release") { |
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.
not sure I understand why this changed
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.
because this test was failing before at the end of the function (line proxy.go:109) since the namespace was different (but not the name). That no longer fails there because we are not manually checking the namespace (we rely in the helm library to behave). The only way to make this fail with the new code is to use a name that doesn't exist. In that case the function fails at proxy.go:99.
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 see, makes sense!
Fixes #704
When getting a release, tell tiller to:
That's the release we return to the user. This way we avoid returning an old release.
Since this is just adding new options to the Helm request we cannot unit-test it. This is because the fake helm client doesn't have into account the given options. I checked that the issue is resolved manually in a cluster with the situation described in #704.