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.

Sign up for GitHub

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

Sort packages by version instead of relying on ChartVersions[0] #6588

Merged
merged 5 commits into from
Aug 9, 2023
Merged

Sort packages by version instead of relying on ChartVersions[0] #6588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7db3478
Export PackageSemVersion
antgamdia Aug 8, 2023
c5f7ef2
Use `sortedVersions[0]` instead of `ChartVersions[0]`
antgamdia Aug 8, 2023
013c131
Fall back to previous behavior if no semver
antgamdia Aug 8, 2023
ab565d7
Add more chart versions in test w/o changes in semantics
antgamdia Aug 8, 2023
6653572
Fix test
antgamdia Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 56 additions & 15 deletions cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,14 @@ func (s *Server) GetAvailablePackageDetail(ctx context.Context, request *connect
return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("Chart returned without any versions: %+v", chart))
}
if version == "" {
version = chart.ChartVersions[0].Version
sortedVersions, err := pkgutils.SortByPackageVersion(chart.ChartVersions)
if err != nil {
// If there was an error parsing a version as semver, fall back to ChartVersions[0]
log.Errorf("error parsing versions as semver: %w", err)
version = chart.ChartVersions[0].Version
} else {
version = sortedVersions[0].Version.String()
}
}
fileID := fileIDForChart(unescapedChartID, version)
chartFiles, err := s.manager.GetChartFiles(namespace, fileID)
Expand Down Expand Up @@ -419,11 +426,22 @@ func AvailablePackageDetailFromChart(chart *models.Chart, chartFiles *models.Cha
pkg.AvailablePackageRef.Context = &corev1.Context{Namespace: chart.Repo.Namespace}
}

// We assume that chart.ChartVersions[0] will always contain either: the latest version or the specified version
// We assume that sortedVersions[0] will always contain either: the latest version or the specified version
if chart.ChartVersions != nil || len(chart.ChartVersions) != 0 {
pkg.Version = &corev1.PackageAppVersion{
PkgVersion: chart.ChartVersions[0].Version,
AppVersion: chart.ChartVersions[0].AppVersion,
sortedVersions, err := pkgutils.SortByPackageVersion(chart.ChartVersions)
if err != nil {
// If there was an error parsing a version as semver, fall back to ChartVersions[0]
log.Errorf("error parsing versions as semver: %w", err)
version = chart.ChartVersions[0].Version
pkg.Version = &corev1.PackageAppVersion{
PkgVersion: chart.ChartVersions[0].Version,
AppVersion: chart.ChartVersions[0].AppVersion,
}
} else {
pkg.Version = &corev1.PackageAppVersion{
PkgVersion: sortedVersions[0].Version.String(),
AppVersion: sortedVersions[0].AppVersion,
}
}
pkg.Readme = chartFiles.Readme
pkg.DefaultValues = chartFiles.DefaultValues
Expand Down Expand Up @@ -523,10 +541,21 @@ func (s *Server) GetInstalledPackageSummaries(ctx context.Context, request *conn
// TODO(agamez): deal with multiple matches, perhaps returning []AvailablePackageRef ?
// Example: global + namespaced repo including an overlapping subset of packages.
if len(charts) > 0 && len(charts[0].ChartVersions) > 0 {
installedPkgSummaries[i].LatestVersion = &corev1.PackageAppVersion{
PkgVersion: charts[0].ChartVersions[0].Version,
AppVersion: charts[0].ChartVersions[0].AppVersion,
sortedVersions, err := pkgutils.SortByPackageVersion(charts[0].ChartVersions)
if err != nil {
// If there was an error parsing a version as semver, fall back to ChartVersions[0]
log.Errorf("error parsing versions as semver: %w", err)
installedPkgSummaries[i].LatestVersion = &corev1.PackageAppVersion{
PkgVersion: charts[0].ChartVersions[0].Version,
AppVersion: charts[0].ChartVersions[0].AppVersion,
}
} else {
installedPkgSummaries[i].LatestVersion = &corev1.PackageAppVersion{
PkgVersion: sortedVersions[0].Version.String(),
AppVersion: sortedVersions[0].AppVersion,
}
}

}
installedPkgSummaries[i].Status = &corev1.InstalledPackageStatus{
Ready: rel.Info.Status == release.StatusDeployed,
Expand Down Expand Up @@ -650,11 +679,21 @@ func (s *Server) GetInstalledPackageDetail(ctx context.Context, request *connect
}
}
if len(charts[0].ChartVersions) > 0 {
cv := charts[0].ChartVersions[0]
installedPkgDetail.LatestVersion = &corev1.PackageAppVersion{
PkgVersion: cv.Version,
AppVersion: cv.AppVersion,
sortedVersions, err := pkgutils.SortByPackageVersion(charts[0].ChartVersions)
if err != nil {
// If there was an error parsing a version as semver, fall back to ChartVersions[0]
log.Errorf("error parsing versions as semver: %w", err)
installedPkgDetail.LatestVersion = &corev1.PackageAppVersion{
PkgVersion: charts[0].ChartVersions[0].Version,
AppVersion: charts[0].ChartVersions[0].AppVersion,
}
} else {
installedPkgDetail.LatestVersion = &corev1.PackageAppVersion{
PkgVersion: sortedVersions[0].Version.String(),
AppVersion: sortedVersions[0].AppVersion,
}
}

}
}

Expand Down Expand Up @@ -901,10 +940,9 @@ func (s *Server) fetchChartWithRegistrySecrets(ctx context.Context, headers http
return nil, nil, connect.NewError(connect.CodeInternal, fmt.Errorf("Unable to fetch the chart %s (version %s) from the namespace %q: %w", chartID, chartDetails.Version, chartDetails.AppRepositoryResourceNamespace, err))
}
var tarballURL string
// If the chart is cached, we can use the tarball URL from the cache,
// we assume cachedChart.ChartVersions only contains 1 element
if cachedChart.ChartVersions != nil && len(cachedChart.ChartVersions) == 1 && cachedChart.ChartVersions[0].URLs != nil {
// The tarball URL will always be the first URL in the repo.chartVersions:
// https://helm.sh/docs/topics/chart_repository/#the-index-file
// /~https://github.com/helm/helm/blob/v3.7.1/cmd/helm/search/search_test.go#L63
tarballURL = chartTarballURL(cachedChart.Repo, cachedChart.ChartVersions[0])
log.InfoS("using chart tarball", "url", tarballURL)
}
Expand Down Expand Up @@ -935,6 +973,9 @@ func (s *Server) fetchChartWithRegistrySecrets(ctx context.Context, headers http
}

func chartTarballURL(r *models.Repo, cv models.ChartVersion) string {
// The tarball URL will always be the first URL, ie. URL[0], in the repo.chartVersions[i]:
// https://helm.sh/docs/topics/chart_repository/#the-index-file
// /~https://github.com/helm/helm/blob/v3.7.1/cmd/helm/search/search_test.go#L63
source := strings.TrimSpace(cv.URLs[0])
parsedUrl, err := url.ParseRequestURI(source)
if err != nil || parsedUrl.Scheme == "" {
Expand Down
36 changes: 18 additions & 18 deletions cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,9 +410,9 @@ func TestGetAvailablePackageSummaries(t *testing.T) {
},
expectDBQueryNamespace: globalPackagingNamespace,
charts: []*models.Chart{
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory),
makeChart("chart-3-global", "repo-1", "http://chart-3", globalPackagingNamespace, []string{"2.0.0"}, DefaultChartCategory),
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory),
makeChart("chart-3-global", "repo-1", "http://chart-3", globalPackagingNamespace, []string{"1.0.0", "2.0.0"}, DefaultChartCategory),
},
expectedResponse: &corev1.GetAvailablePackageSummariesResponse{
AvailablePackageSummaries: []*corev1.AvailablePackageSummary{
Expand Down Expand Up @@ -478,8 +478,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) {
},
expectDBQueryNamespace: "my-ns",
charts: []*models.Chart{
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory),
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory),
},
expectedResponse: &corev1.GetAvailablePackageSummariesResponse{
AvailablePackageSummaries: []*corev1.AvailablePackageSummary{
Expand Down Expand Up @@ -530,8 +530,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) {
},
expectDBQueryNamespace: globalPackagingNamespace,
charts: []*models.Chart{
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory),
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory),
},
expectedResponse: &corev1.GetAvailablePackageSummariesResponse{
AvailablePackageSummaries: []*corev1.AvailablePackageSummary{
Expand Down Expand Up @@ -621,8 +621,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) {
},
expectDBQueryNamespace: globalPackagingNamespace,
charts: []*models.Chart{
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory),
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory),
makeChart("chart-3", "repo-1", "http://chart-3", "my-ns", []string{"1.0.0"}, DefaultChartCategory),
},
expectedResponse: &corev1.GetAvailablePackageSummariesResponse{
Expand Down Expand Up @@ -665,8 +665,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) {
},
expectDBQueryNamespace: globalPackagingNamespace,
charts: []*models.Chart{
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, DefaultChartCategory),
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, DefaultChartCategory),
makeChart("chart-3", "repo-1", "http://chart-3", "my-ns", []string{"1.0.0"}, DefaultChartCategory),
},
expectedResponse: &corev1.GetAvailablePackageSummariesResponse{
Expand Down Expand Up @@ -718,8 +718,8 @@ func TestGetAvailablePackageSummaries(t *testing.T) {
},
expectDBQueryNamespace: "my-ns",
charts: []*models.Chart{
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"3.0.0"}, "foo"),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"2.0.0"}, "bar"),
makeChart("chart-1", "repo-1", "http://chart-1", "my-ns", []string{"2.0.0", "3.0.0"}, "foo"),
makeChart("chart-2", "repo-1", "http://chart-2", "my-ns", []string{"1.0.0", "2.0.0"}, "bar"),
makeChart("chart-3", "repo-1", "http://chart-3", "my-ns", []string{"1.0.0"}, "bar"),
},
expectedResponse: &corev1.GetAvailablePackageSummariesResponse{
Expand Down Expand Up @@ -852,7 +852,7 @@ func TestAvailablePackageDetailFromChart(t *testing.T) {
}{
{
name: "it returns AvailablePackageDetail if the chart is correct",
chart: makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"3.0.0"}, DefaultChartCategory),
chart: makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory),
chartFiles: &models.ChartFiles{
Readme: "chart readme",
DefaultValues: "chart values",
Expand Down Expand Up @@ -885,7 +885,7 @@ func TestAvailablePackageDetailFromChart(t *testing.T) {
},
{
name: "it includes additional values files in AvailablePackageDetail when available",
chart: makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"3.0.0"}, DefaultChartCategory),
chart: makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory),
chartFiles: &models.ChartFiles{
Readme: "chart readme",
DefaultValues: "chart values",
Expand Down Expand Up @@ -972,7 +972,7 @@ func TestGetAvailablePackageDetail(t *testing.T) {
Identifier: "repo-1%2Ffoo",
},
},
charts: []*models.Chart{makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"3.0.0"}, DefaultChartCategory)},
charts: []*models.Chart{makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"2.0.0", "3.0.0"}, DefaultChartCategory)},
expectedPackage: &corev1.AvailablePackageDetail{
Name: "foo",
DisplayName: "foo",
Expand Down Expand Up @@ -1007,7 +1007,7 @@ func TestGetAvailablePackageDetail(t *testing.T) {
},
PkgVersion: "1.0.0",
},
charts: []*models.Chart{makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"3.0.0", "2.0.0", "1.0.0"}, DefaultChartCategory)},
charts: []*models.Chart{makeChart("foo", "repo-1", "http://foo", "my-ns", []string{"1.0.0", "2.0.0", "3.0.0"}, DefaultChartCategory)},
expectedPackage: &corev1.AvailablePackageDetail{
Name: "foo",
DisplayName: "foo",
Expand Down Expand Up @@ -1204,7 +1204,7 @@ func TestGetAvailablePackageVersions(t *testing.T) {
},
{
name: "it returns the package version summary",
charts: []*models.Chart{makeChart("apache", "bitnami", "http://apache", "kubeapps", []string{"3.0.0", "2.0.0", "1.0.0"}, DefaultChartCategory)},
charts: []*models.Chart{makeChart("apache", "bitnami", "http://apache", "kubeapps", []string{"2.0.0", "3.0.0", "1.0.0"}, DefaultChartCategory)},
request: &corev1.GetAvailablePackageVersionsRequest{
AvailablePackageRef: &corev1.AvailablePackageReference{
Context: &corev1.Context{
Expand Down
35 changes: 23 additions & 12 deletions cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,13 @@ func GetDefaultVersionsInSummary() VersionsInSummary {
return defaultVersionsInSummary
}

type packageSemVersion struct {
type PackageSemVersion struct {
*semver.Version
appVersion string
AppVersion string
}

func sortByPackageVersion(versions []models.ChartVersion) ([]*packageSemVersion, error) {
var sortedVersions []*packageSemVersion
func SortByPackageVersion(versions []models.ChartVersion) ([]*PackageSemVersion, error) {
var sortedVersions []*PackageSemVersion
for _, v := range versions {
version, err := semver.NewVersion(v.Version)
if err != nil {
Expand All @@ -70,9 +70,9 @@ func sortByPackageVersion(versions []models.ChartVersion) ([]*packageSemVersion,
return nil, fmt.Errorf("Chart version %q is not semver", v.Version)
}

sortedVersions = append(sortedVersions, &packageSemVersion{
sortedVersions = append(sortedVersions, &PackageSemVersion{
Version: version,
appVersion: v.AppVersion,
AppVersion: v.AppVersion,
})
}
sort.Slice(sortedVersions, func(i, j int) bool {
Expand All @@ -87,7 +87,7 @@ func PackageAppVersionsSummary(versions []models.ChartVersion, versionInSummary
var pav []*corev1.PackageAppVersion

// Sort versions
sortedVersions, err := sortByPackageVersion(versions)
sortedVersions, err := SortByPackageVersion(versions)
if err != nil {
// If there was an error parsing a version as semver, we log the error
// and simply return the versions, as Helm does.
Expand Down Expand Up @@ -127,7 +127,7 @@ func PackageAppVersionsSummary(versions []models.ChartVersion, versionInSummary
// Include the version and update the version map.
pav = append(pav, &corev1.PackageAppVersion{
PkgVersion: version.Version.String(),
AppVersion: version.appVersion,
AppVersion: version.AppVersion,
})

if _, ok := versionMap[version.Major()]; !ok {
Expand All @@ -154,7 +154,7 @@ func IsValidChart(chart *models.Chart) (bool, error) {
return false, connect.NewError(connect.CodeInternal, fmt.Errorf("required field .Repo not found on helm chart: %v", chart))
}
if chart.ChartVersions == nil || len(chart.ChartVersions) == 0 {
return false, connect.NewError(connect.CodeInternal, fmt.Errorf("required field .chart.ChartVersions[0] not found on helm chart: %v", chart))
return false, connect.NewError(connect.CodeInternal, fmt.Errorf("required field .chart.ChartVersions not found on helm chart or is empty: %v", chart))
} else {
for _, chartVersion := range chart.ChartVersions {
if chartVersion.Version == "" {
Expand Down Expand Up @@ -197,15 +197,26 @@ func AvailablePackageSummaryFromChart(chart *models.Chart, plugin *plugins.Plugi
pkg.AvailablePackageRef.Context = &corev1.Context{Namespace: chart.Repo.Namespace}

if chart.ChartVersions != nil || len(chart.ChartVersions) != 0 {
pkg.LatestVersion = &corev1.PackageAppVersion{
PkgVersion: chart.ChartVersions[0].Version,
AppVersion: chart.ChartVersions[0].AppVersion,
sortedVersions, err := SortByPackageVersion(chart.ChartVersions)
if err != nil {
// If there was an error parsing a version as semver, fall back to ChartVersions[0]
log.Errorf("error parsing versions as semver: %w", err)
pkg.LatestVersion = &corev1.PackageAppVersion{
PkgVersion: chart.ChartVersions[0].Version,
AppVersion: chart.ChartVersions[0].AppVersion,
}
} else {
pkg.LatestVersion = &corev1.PackageAppVersion{
PkgVersion: sortedVersions[0].Version.String(),
AppVersion: sortedVersions[0].AppVersion,
}
}
}

return pkg, nil
}

// TODO(agamez): I have replaced chart.ChartVersions[0] with SortByPackageVersion(...)[0]
// TODO @gfichtenholt: I really wanted to put helm plugin's implementation of AvailablePackageDetailFromChart()
// here, and use it in flux plugin as well. But I found out a couple of flaws in the implementation and decided
// against it. Namely:
Expand Down
4 changes: 2 additions & 2 deletions cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,9 +542,9 @@ func TestAvailablePackageSummaryFromChart(t *testing.T) {
},
Maintainers: []chart.Maintainer{{Name: "me", Email: "me@me.me"}},
ChartVersions: []models.ChartVersion{
{Version: "3.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"},
{Version: "2.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"},
{Version: "1.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"},
{Version: "2.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"},
{Version: "3.0.0", AppVersion: DefaultAppVersion, Readme: "chart readme", DefaultValues: "chart values", Schema: "chart schema"},
},
},
expected: &corev1.AvailablePackageSummary{
Expand Down