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

Fix get release #781

Merged
merged 3 commits into from
Nov 1, 2018
Merged

Conversation

migmartri
Copy link
Contributor

@migmartri migmartri commented Nov 1, 2018

Changes the way we retrieve a single release name. Instead of listing all the releases and retrieving the last one, we use what Helm itself uses instead.

I have also imported a helper to extract error messages from GRPC so we can imitate what helm does without the need of having extra error handling.

// curl tiller-proxy...
{
  "code": 404,
  "message": "release: \"wordpress-miguel23\" not found"
}

Also note that this method does not require passing a namespace so changes will be made in further PRs

Refs #765

Copy link
Contributor

@prydonius prydonius left a comment

Choose a reason for hiding this comment

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

Does the test need to be changed as the error message is different now?

@migmartri migmartri merged commit 7d5e8bf into vmware-tanzu:master Nov 1, 2018
@migmartri
Copy link
Contributor Author

Does the test need to be changed as the error message is different now?

No, this is because the returned error by GRPC matches the previous one.

I needed to change the updateRelease code though to remove the specific error handling for 404, which I am not sure why it is needed though since the UI takes into account the code returned not the message.

@migmartri migmartri mentioned this pull request Nov 1, 2018
helm.ReleaseListSort(int32(services.ListSort_LAST_RELEASED)),
helm.ReleaseListOrder(int32(services.ListSort_DESC)),
)
release, err := p.helmClient.ReleaseContent(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good find. I am checking the current helm code and it seems that the signature of the function is different:

https://github.com/helm/helm/blob/master/cmd/helm/get.go#L90

Is that because a newer version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it? I believe that it is the same. It seems to be a variadic function that accepts zero or more options

	ReleaseContent(rlsName string, opts ...ContentOption) (*rls.GetReleaseContentResponse, error)

In Helm's get CMD it passes the version but looking at the code I noticed that if you do not pass the version, the code returns the last one which is what we want https://github.com/helm/helm/blob/7cad59091a9451b2aa4f95aa882ea27e6b195f98/pkg/tiller/release_content.go#L32

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I didn't check the function definition

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants