[Helm3] Return chart version and metadata when listing #1441
Conversation
pkg/agent/agent.go
Outdated
| ChartMetadata: chartv1.Metadata{ | ||
| Name: r.Chart.Metadata.Name, | ||
| Home: r.Chart.Metadata.Home, | ||
| Sources: r.Chart.Metadata.Sources, | ||
| Version: r.Chart.Metadata.Version, | ||
| Description: r.Chart.Metadata.Description, | ||
| Keywords: r.Chart.Metadata.Keywords, | ||
| Icon: r.Chart.Metadata.Icon, | ||
| ApiVersion: r.Chart.Metadata.APIVersion, | ||
| Tags: r.Chart.Metadata.Tags, | ||
| AppVersion: r.Chart.Metadata.AppVersion, | ||
| }, |
There was a problem hiding this comment.
I think we could and should re-use compatibleMetadata. I managed to make it typecheck with little effort at least.
This would of course mean said function would have to be moved to somewhere in pkg. At closer thought, maybe it makes more sense to put the entire dashboardcompat.go in pkg. It really is more of a package than a command.
There was a problem hiding this comment.
That's a good point. At the end we have a package to convert releases from one format to the other so we can reuse it.
pkg/agent/agent_test.go
Outdated
| Version: "2", | ||
| Version: "1.0.0", |
There was a problem hiding this comment.
Is this change trivially correct? If not, maybe add a note in the PR?
There was a problem hiding this comment.
The version is irrelevant here but I can change the number so the title still applies 👍
| Version: "2", | ||
| Version: "1.0.0", |
There was a problem hiding this comment.
Is this change trivially correct? If not, maybe add a note in the PR?
There was a problem hiding this comment.
In this case it's not needed (the version is not relevant either)
pkg/agent/agent.go
Outdated
| Namespace: r.Namespace, | ||
| Status: r.Info.Status.String(), | ||
| func appOverviewFromRelease(r *release.Release) (proxy.AppOverview, error) { | ||
| releaseHelm2, err := helm3to2.Convert(*r) |
There was a problem hiding this comment.
Converting the release to a helm 2 release seems unnecessary (it's only used for the maintainers) and also the only reason for requiring an error in the signature of this function?
That is, if helm3to2.ConvertMaintainers(r.Chart.Metadata.Maintainers) could be used below instead (ie. just making compatableMaintainers public), then we don't need to handle the error at all (nor convert the whole release).
Fine either way, just a thought.
There was a problem hiding this comment.
Well, we need the whole metadata (not just the maintainers) so the function to expose would be compatibleMetadata but yes, we could do that. I wanted to to keep the methods exposed by the compatibility package as minimum as possible but there is not a good reason for not exposing the metadata conversion.
There was a problem hiding this comment.
I for one see no problem with exporting helm3to2 functions at will. They're all supposed to be pure and reusable anyway.
There was a problem hiding this comment.
Although one must keep in mind that some of the type signatures in helm3to2 are essentially lies, since it's impossible in general to write a function from Helm 2 releases to Helm 3 releases.
Description of the change
When listing apps, the expected result (
AppOverview) is expected to have the chart version and the chart metadata included in the response. This is used to build hte AppList view, showing updates if necessary.See the tiller-proxy implementation:
https://github.com/kubeapps/kubeapps/blob/8bb060e15ad315c1561d0133cbd30827d292dcd2/pkg/proxy/proxy.go#L216
Benefits
Before this change:
After the change:
Additional information
cc/ @latiif @SimonAlling