[kubeapps-apis] Add direct-helm GetAvailablePackageDetail #3034
Conversation
Conflicts: chart/kubeapps/Chart.yaml cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go
antgamdia
changed the title
You can target your other branch in situations like this, so reviews can go ahead with just the relevant diff. It's a bit of a pain, but works OK. When the preceding branch lands, GH will automatically update the target of this PR back to the main branch, but you still often need to rebase, removing the extra commits. |
…ePackageSummaries
Conflicts: cmd/assetsvc/handler.go cmd/assetsvc/handler_test.go cmd/assetsvc/main.go cmd/assetsvc/pkg/assetsvc_utils/postgresql_utils.go cmd/assetsvc/pkg/assetsvc_utils/postgresql_utils_test.go cmd/assetsvc/pkg/assetsvc_utils/utils.go cmd/assetsvc/pkg/utils/postgresql_utils.go cmd/assetsvc/pkg/utils/postgresql_utils_test.go cmd/assetsvc/pkg/utils/utils.go cmd/assetsvc/postgres_db_test.go cmd/assetsvc/postgresql_utils.go cmd/assetsvc/postgresql_utils_test.go cmd/assetsvc/utils.go cmd/kubeapps-apis/gen/core/packages/v1alpha1/packages.pb.go cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go
antgamdia
force-pushed
the
2993-helmPlugin-GetAvailablePackageDetail
branch
from
d1ad0b0
to
d278b12
Compare
Conflicts: cmd/kubeapps-apis/docs/kubeapps-apis.swagger.json cmd/kubeapps-apis/gen/core/packages/v1alpha1/packages.pb.go cmd/kubeapps-apis/gen/plugins/helm/packages/v1alpha1/helm.pb.gw.go cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go cmd/kubeapps-apis/proto/kubeappsapis/core/packages/v1alpha1/packages.proto
| // After requesting a specific namespace, we have to ensure the user can actually access to it | ||
| // If checking the global namespace, allow access always | ||
| hasAccess := namespace == s.globalPackagingNamespace | ||
| if !hasAccess { | ||
| var err error | ||
| // If checking another namespace, check if the user has access (ie, "get secrets in this ns") | ||
| hasAccess, err = s.hasAccessToNamespace(ctx, namespace) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "Unable to check if the user has access to the namespace: %s", err) | ||
| } | ||
| if !hasAccess { | ||
| // If the user has not access, return a unauthenticated response, otherwise, continue | ||
| return nil, status.Errorf(codes.Unauthenticated, "The current user has no access to the namespace %q", namespace) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can this now be dry'd up if it's the same in both this function and the summaries one?
|
|
||
| if chart.Description == "" { | ||
| return nil, status.Errorf(codes.Internal, "required field .Description not found on helm package: %v", chart) | ||
| } |
There was a problem hiding this comment.
Is the description required? It's listed as optional at https://helm.sh/docs/topics/charts/#the-chartyaml-file
There was a problem hiding this comment.
I forgot to verify it! Last week I thought: ok, let's put every param as mandatory and later on I'll look them up in the helm code or website... but...ouch, I never did.
Thanks for the pointer!
| return nil, status.Errorf(codes.Internal, "required field .Description not found on helm package: %v", chart) | ||
| } | ||
| pkg.ShortDescription = chart.Description | ||
| pkg.LongDescription = chart.Description |
There was a problem hiding this comment.
I think we should just leave LongDescription blank for helm charts, which only have a single description field in the Chart.yaml? If we copy it like this, a client would display both, I'd think?
There was a problem hiding this comment.
Ah, that makes sense. I just set both so that we can decide later. But, we better avoid duplicated fields :P
| pkg.Name = chart.Name | ||
| pkg.DisplayName = chart.Name | ||
|
|
||
| if chart.Icon == "" { |
There was a problem hiding this comment.
Also listed as optional at https://helm.sh/docs/topics/charts/#the-chartyaml-file
| pkg.ShortDescription = chart.Description | ||
| pkg.LongDescription = chart.Description | ||
|
|
||
| if chart.Maintainers == nil { |
There was a problem hiding this comment.
Same, not required at https://helm.sh/docs/topics/charts/#the-chartyaml-file
| } | ||
| pkg.PkgVersion = chart.ChartVersions[0].Version | ||
|
|
||
| if chart.ChartVersions[0].AppVersion == "" { |
There was a problem hiding this comment.
AppVersion is also optional at https://helm.sh/docs/topics/charts/#the-chartyaml-file so I assume it can be optional in our cache.
| } | ||
| pkg.AppVersion = chart.ChartVersions[0].AppVersion | ||
|
|
||
| if chart.ChartVersions[0].Readme == "" { |
There was a problem hiding this comment.
Readme file is optional at https://helm.sh/docs/topics/charts/#the-chart-file-structure
| } | ||
| pkg.DefaultValues = chart.ChartVersions[0].Values | ||
|
|
||
| if chart.ChartVersions[0].Schema == "" { |
There was a problem hiding this comment.
values.schema.json also optional at https://helm.sh/docs/topics/charts/#the-chart-file-structure
| name: "it returns internal error if chart is invalid", | ||
| chart: &models.Chart{Name: "foo"}, | ||
| statusCode: codes.Internal, | ||
| }, |
There was a problem hiding this comment.
I guess you could add failing tests here to show that it should succeed without certain fields (ie. readme, icon, maintainers) then fix it for them to pass :)
There was a problem hiding this comment.
TDD is great for these fixes :P Added a TestIsValidChart test suite
|
Great, good to go when the conflict is resolved. |
Description of the change
This PR just implements the
GetAvailablePackageDetailmethod in the direct-helm plugin.Benefits
We will have a brand new
GetAvailablePackageDetailoperation up and running.Possible drawbacks
N/A
Applicable issues
Additional information
(Draft until we merge the other PRs so that it becomes easier to review; didn't want to push to the repo)