Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

[WIP] Adding Search functions for Helm Client #623

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattfarina
Copy link
Contributor

This API provides a simplified response without any pointers back to
other Monocular APIs. It is meant for the Helm v3 client.

This is a work in progress and posted now to get general guidance and to prompt discussion before continuing.

This API provides a simplified response without any pointers back to
other Monocular APIs. It is meant for the Helm v3 client.

Signed-off-by: Matt Farina <[email protected]>
@mattfarina mattfarina requested a review from prydonius April 23, 2019 19:40
@prydonius
Copy link
Member

Thanks for mocking up an example of what this might look like, but I feel like I'm missing some context here. I would love to understand more about the Helm client's requirements around search, is there a writeup that describes how the client needs to interact with the API and why the current API is not suitable?

@mattfarina
Copy link
Contributor Author

@prydonius Here is some additional context.

@adamreese if there is anything I missed from our conversation please feel free to add it.

The current URL path to search in Monocular is /api/chartsvc/v1/charts/search?q=[term]. The response contains relative paths, like /v1/assets/banzaicloud-stable/mysql/logo-160x160-fit.png, to the chartsvc. This is because chartsvc is a separate microservice from the UI and ingress maps it to a URL path. chartsvc generates relative paths to itself. Because the UI client and chartsvc are part of Monocular we know what all of this means and how to navigate the paths.

Let's consider making this a public API for outside clients and what that might mean:

  • We won't want to leak our implementation to search. We've already talked about a path like /api/v1/search to expose externally. If we do this the relative paths in the response are not to anything an end user will expect to know.
  • If we keep the current search path it's leaking our implementation and we have to support a URL there for as long as it's in use by Helm clients (even those not in the helm cli). That's not idea either. And, people who use the API need to learn that the relative paths aren't relative to the URL path but a sub-path. Not a great UX.
  • The current response as a lot of nesting. I counted depths of 5 levels along with numerous different objects. In monocular this is handled with interface{} quite a bit. For a Go client to pull this in and work with the data (some of the needed data is nested) means we will have to have numerous structs to represent the search. The idea here is the output is causing clients to have additional complexity. Can we reduce this?
  • Most of the data in the response isn't needed for an external search. It's the right about if you need to build a UI showcasing everything, like we do with the front-end. For helm search it's a lot of extra information.

At the end of this I was looking for a way to craft a good external facing API with a workable developer experience.

Thoughts?

@prydonius
Copy link
Member

Thanks for adding additional context. My main concern here would be to avoid creating a separate API if we can and rather make some changes to make this a better external-facing API.

We won't want to leak our implementation to search. We've already talked about a path like /api/v1/search to expose externally. If we do this the relative paths in the response are not to anything an end user will expect to know.

I think we could mount the chartsvc API at the root and drop the chartsvc prefix. If we do that, it will be /api/v1/charts/search and /api/v1/assets. If the client knows that the base API endpoint is hub.helm.sh/api/ then it should be able to build the relative links. I think that should be sufficient (users will need to know it's under /api, but not have to know about /api/chartsvc), and we can also document it. Also given the Helm CLI usecase doesn't need the assets endpoint, we don't really need to worry about this too much for now. If we find other users of the public API having difficulty working around this, I'm happy to change it. Does that sound reasonable?

The current response as a lot of nesting. I counted depths of 5 levels along with numerous different objects. In monocular this is handled with interface{} quite a bit. For a Go client to pull this in and work with the data (some of the needed data is nested) means we will have to have numerous structs to represent the search. The idea here is the output is causing clients to have additional complexity. Can we reduce this?

We went with the JSON API spec initially, which indeed creates some unnecessary nesting. The first level (data and meta) is necessary for pagination and other meta info we might need to have in the API. data is then just an array of JSON API objects, and attributes for each object mostly follows the Chart.yaml interface. relationships will also likely need to be read by the Helm CLI to get the latest version for each Chart. I would try to see if this is workable before flattening the API, otherwise I suggest proposing a v2 API that we can work towards adopting everywhere.

Most of the data in the response isn't needed for an external search. It's the right about if you need to build a UI showcasing everything, like we do with the front-end. For helm search it's a lot of extra information.

Agreed, but I'm not sure that it warrants having to maintain a separate API. The cost of sending down this additional data is fairly low, right? Also, as a public external API, it might be useful to have extra information that other clients can now use.

@prydonius prydonius mentioned this pull request May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants