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.

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

Enable viewing AvailablePackageDetail with flux. #3628

Merged
merged 2 commits into from
Oct 24, 2021
Merged

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Oct 21, 2021

Description of the change

The current Kubeapps dashboard UX, in particular the urls, support only two options for where to find an available package: the current namespace or the global one.

This doesn't work for flux, where (currently) a repo can be in any namespace and is effectively public.

This PR updates the URLs for displaying an available package so that the package namespace (and cluster) is always included in the URL (ie. we're representing the full available package reference in the URL now) and enables viewing available packages for a target namespace (where those packages are from a repo in a different namespace).

CI will tell if this breaks current helm support :)

Benefits

I can view the available package detail when using the flux plugin.

Possible drawbacks

Applicable issues

Ref: #3610

Additional information

Just noted that fetching the available package detail appears to be taking around 2.5s (see below). Not a big deal, but just checking in case @gfichtenholt can make it quicker (he's good at that :P )

flux-apache

Note also: the screenshot shows that the chart title is being rendered without being decoded, but I'm guessing that's from your PR @antgamdia ? Let me know if not, but don't think I've changed the chart id here.

@absoludity absoludity marked this pull request as draft October 21, 2021 05:49
@gfichtenholt
Copy link
Contributor

gfichtenholt commented Oct 21, 2021

Just noted that fetching the available package detail appears to be taking around 2.5s (see below). Not a big deal, but just checking in case @gfichtenholt can make it quicker (he's good at that :P )

do you happen to know if is it taking this long on the first call (for a given package) or subsequent calls too? My magic does have limits :-) Ah never mind, checked my code. I know why that is. I'm not caching any specific charts (only parsed repo index). (Some of the) chart details have to come from a tarball, so if one isn't already there I create a HelmChart on-the-fly, wait while until it reconciles, get the tarball, and then clean up by deleting the HelmChart. I'm a little surprised that it takes that long, I have seen much shorter times, bit I digress. So 2nd call wouldn't be any faster than the 1st. If I didn't do the clean up, it obviously would be, but I didn't want potentially unbound number of HelmCharts laying around after a while.

One obvious solution would be to cache package details (i.e. introduce another resource-based watcher cache)...

Another solution might be to make the cleanup process a bit more sophisticated, to make it kind of a garbage collector, that keeps around a around , say, 5 of LRU HelmCharts. That way the number of HelmCharts left around is still bounded. Back in school they taught me if a user clicks on one package, they're likely to click there again, so this would solve it too.

Neither of these approaches would not speed up the 1st call, but either one would make the cost of 2nd and subsequent calls to GetAvailablePackageDetail insignificant.

In any case @absoludity I think this is a legit issue in flux plugin that merits fixing at some point. Would you file a separate issue for that? Feel free to assign to me. Thx

Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity marked this pull request as ready for review October 22, 2021 02:01
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, I did have a look at it and thought I already +1ed it :P
Thanks for the changes!

@absoludity
Copy link
Contributor Author

Ooops, I did have a look at it and thought I already +1ed it :P Thanks for the changes!

Thanks :)

@absoludity absoludity merged commit 00c8395 into master Oct 24, 2021
@absoludity absoludity deleted the flux-ui-3 branch October 24, 2021 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants