Update getPaginatedChartList to use namespace. #1562
Conversation
cmd/assetsvc/handler.go
Outdated
| @@ -131,9 +131,10 @@ func listCharts(w http.ResponseWriter, req *http.Request) { | |||
| } | |||
|
|
|||
| // listRepoCharts returns a list of charts in the given repo | |||
| // TODO: mnelson: check with Andres why we need a separate function above when `params["repo"]` should be "" by default | |||
There was a problem hiding this comment.
we don't need it now since both functions now require "params". Before they were different functions to make it clear that listCharts didn't require any param.
cmd/assetsvc/main.go
Outdated
| apiv1.Methods("GET").Path("/assets/{repo}/{chartName}/versions/{version}/README.md").Handler(WithParams(getChartVersionReadme)) | ||
| apiv1.Methods("GET").Path("/assets/{repo}/{chartName}/versions/{version}/values.yaml").Handler(WithParams(getChartVersionValues)) | ||
| apiv1.Methods("GET").Path("/assets/{repo}/{chartName}/versions/{version}/values.schema.json").Handler(WithParams(getChartVersionSchema)) | ||
| // TODO: Check if we actually need multiple handlers here or can just use one path per endpoint. |
There was a problem hiding this comment.
I see changes in all these lines but I am actually not seeing any change 🤔 I guess you are just changing the call to listCharts to add WithParams but I am not sure.
BTW potentially answering your TODO, it's necessary to repeat the Handler every time there is a new Query param
There was a problem hiding this comment.
I see changes in all these lines but I am actually not seeing any change thinking I guess you are just changing the call to listCharts to add WithParams but I am not sure.
The new versions are prefixed with /ns/{namespace} in the paths.
BTW potentially answering your TODO, it's necessary to repeat the Handler every time there is a new Query param
Why is that? I mean, if we have one path here which defines all possible query params, and one handler which handles that (including if those params are empty)?
There was a problem hiding this comment.
Why is that? I mean, if we have one path here which defines all possible query params, and one handler which handles that (including if those params are empty)?
The matches were tricky. I am not sure if a request will match if there is a query parameter missing. In any case, I think that defining the query params in the path is not required so if you only have one backend function you can just try to read those params in the function.
| if repo != "" { | ||
| pipeline = append(pipeline, bson.M{"$match": bson.M{"repo.name": repo}}) | ||
| matcher["repo.name"] = repo |
There was a problem hiding this comment.
hmm, I see you are removing the "$match": ... I guess it's fine if this means that the match should be exact
There was a problem hiding this comment.
Nope, that was a mistake. I meant to include it in the created pipeline. Updated, thanks.
absoludity
force-pushed
the
1521-send-namespace-assetsvc
branch
from
e1e735d to
2beffbe
Compare
* Update GetChart to inlude namespace * Add namespace to getChartVersion * Add filtered queries * Remove TODO comment * Remove searchCharts handler and functionality as it appears not to be used in the frontend at all.
absoludity
force-pushed
the
1521-send-namespace-assetsvc
branch
from
2acd502 to
fab345a
Compare
|
Let me know if you need me to re-review this |
Thanks, no, I'm just merging the other branches that you approve into this one which I'll then land. |
* Update assetsvc URIs to be namespaced. * Delete unused function. * Update listWithFilters to include namespace * Return namespaced self-links in responses. (#1574)
* Update assetsvc chart test for namespaced URIs * Remove unnecessary handler (#1581) * Disable helm tests unless testing latest release.
This PR cannot land without the related change to the handlers nor updating the frontend to always send the namespace (following PR).
Updates getPaginatedChartList to expect namespace with a value or "_all" and filter accordingly. Adds actual tests for postgres, but not mongodb.
Ref #1521