Add 'GetInstalledPackageSummaries' impl

[antgamdia]

Description of the change

Initial implementation of the GetInstalledPackageSummaries operation

Benefits

An initial and very limited Carvel support in Kubeapps

Possible drawbacks

N/A

Applicable issues

• related Carvel package support in Kubeapps #3221

Additional information

N/A

[absoludity]

Just a typo above

Suggested change

	// GetInstalledPackageSummaries returns the installed packagesmanaged by the 'kapp_controller' plugin
	// GetInstalledPackageSummaries returns the installed packages managed by the 'kapp_controller' plugin

[absoludity]

BTW: I just use the suggested change feature because it clearly shows what is meant in many situations better than words do, but feel free to just do the changes yourself (ie. no-one cares if you commit the suggestion or just do the changes yourself as you move through :) )

[antgamdia]

Thank you for catching the typo! I also use this feature the same way; it's a great manner of showing others what you suggest. When it's an isolated change, I rather apply the suggestion, but in this case, as I have to move through different parts and branches it feels more natural doing the changes myself.
As usual, thanks for your kind and detailed explanations :P

[absoludity]

We should not need these conditions. We had them originally because flux wasn't/isn't supporting multi-cluster, but the client always sends the cluster and we use it in the gateway urls. I'd prefer to use InvalidArgument if the request does not contain the full context. But open to changing that if you've a good reason?

[antgamdia]

As I mentioned in #3784 (comment), let me leave it as is right now. I'm adding a card/issue to discuss what should be doing regarding this. I mean, I'm ok throwing an error, but I'd like to verify first this choose is uniform and consistent with the rest of the functions/plugins.

[absoludity]

We should (at least eventually) be paginating this request, rather than requesting everything every time. Not a priority now though (bigger fish to fry :P (more important things to do right now ))

[absoludity]

Why is the repository relevant here? With carvel I thought the repository is just syncing/updating the Package and PackageMetadata CRs... why would we need the repository CR here? Or maybe you were just meaning the package metadata?

[antgamdia]

Ah, it was me that removed a PackageRepository (therefore deleting the associated Package and PackageMetadata ) and noticed that behavior. From a UX point of view, if a user decides to delete the, let's say, the Bitnami repo, but they still have a package installed, if we throw an error here... the package won't be manageable by kubeapps anymore; forcing the user to manually delete it.

What we did in the past for Helm is return the data we possibly can retrieve and then, the UI will decide which actions will be displayed to users. Currently, we say sth like: "ey, we don't have enough info about this pkg, you only can delete it"

So the point of this ToDo is just to keep in mind this possible use case, so that we can later decide what to do.

[absoludity]

Regarding the PackageMetadata, it's definitely an error if it is not present for a given Package.

[antgamdia]

Same as in #3787 (comment), currently returning error though. It is something we can decide after when we polish up the UI/UX.

[absoludity]

Yeah. You'd normally avoid multiple go-routines writing to the same slice and instead collect the processed data via a channel as you've noted. But I know you're doing it carefully here so that each go routine is responsible for one index only. Stepping back, I'm not sure why the nil's here would be an issue for you anyway given that we should return an error (and a nil value) if an error has been returned, I'd think?

[antgamdia]

Yes, I'd have liked to dig into the goroutines+channels (it's one of my pending topics in Go; btw, now I have a lot of interesting Greg's code to learn from, so thank you!) but I didn't want to spend more time here (I mean, before landing something we can start iterating from).

given that we should return an error (and a nil value) if an error has been returned, I'd think?

That's the point, I'm not so sure we should return a "nil, error" if a goroutine (=single package) fails.
See #3784 (comment)

I'm ok reverting back to the hard-error way, but also happy to discuss longer-term ideas like letting users decide or at least returning "values,error", so that the client/UI was able to decide.

[absoludity]

I'd normally reserve the get prefix for a function if it is getting/fetching something... ie something that is network constrained. What you're doing here is creating an installed package summary based on data that's already been fetched (ie. no network here). Maybe installedPackageSummaryFromData or something without get would be clearer.

[antgamdia]

Yep, I'm with the Java folks who like using getters everywhere, but sounds fair :P However, I feel like I do need a verb for a function name, so... what about buildInstalledPackageSummary(... ?

[absoludity]

I wonder if this is one of the things causing the slowness... some of the SVG's are quite big (300k) and we're having to copy them to each and return via the network (rather than a URL to a resource). If so, definitely a place where caching may help us.

In the mean-time, it might be worth replacing the fmt.Sprintf here with a https://pkg.go.dev/strings#Builder so that we're not copying the string when assigned below. Instead we can just using builder.String() in the final struct below?

[absoludity]

Another thought: if we find that it is the collection of all the svg data being fetched across the network for each installed package detail and sent all together in the one response that is the issue, we could even add an extra endpoint just to this kapp-controller plugin for AvailablePackageIcon, expose it via the gateway, then populate the IconURL of the summary request with the gateway endpoint. This would mean that the response to GetInstalledPackageSummaries (and GetAvailablePackageSummaries) would just have a normal URL for the icon which would be fetched separately by the browser.

We'd still want to avoid pulling the icon data over the network from the k8s server when getting the metadata for other fields though. Have to see if that's possible.

[absoludity]

I don't think this is necessarily true, is it? (Maybe leave at least a comment). I mean, if the PackageInstall has a versionSelection.constraint of 2.0.0, and versions has a 3.0.0, 2.1.0 and 2.0.0, then the latest matching version is still 2.0.0, isn't it?

[antgamdia]

Yes, absolutely; I added a temporary value for populating the field, but just forgot to add a comment explaining it 😅
Adding a TODO for now.