Initial implementation of the remaining core apis operations

[antgamdia]

Description of the change

This PR brings an initial implementation for the already discussed and agreed operations. I've been testing these APIs in the frontend to ensure everything works as expected after the change (helm->core).
Also, I've created an initial test suite for these operations, but I'm having some issues I wanna discuss later.

Benefits

Core (plugin-agnostic) APIs finally!

Possible drawbacks

I still have to check everything works as we expect once we enable the Flux plugin.

Applicable issues

• related Implement remaining parts for the Helm-direct plugin #3340
• closes Implement core APIs for the existing operations #3402

Additional information

Marking it as a draft as I still have to implement the recently added CreateInstalledPackage

Also, I'm having troubles with the tests; let's discuss it offline, but I'd some advice on how to fake the loaded plugins for each test case. Also, I'm unsure of the scope of these tests (ex: whether we have to fake each plugin response (aka, faking postgres, faking redis etc...)

[absoludity]

// TODO: pagination support (we still need to update the existing pagination in helm to use item offsets rather than page offsets to do this)

[antgamdia]

Indeed. In fact, we need GetAvailablePackagesSummaries to be paginated; otherwise, the catalog just won't work (I mean, well fetch the first n elements).
I've implemented a dirty initial way just fetching all the plugin's responses w/o pagination, and then, paginate the aggregated slice.

[absoludity]

Yeah, OK. Good for now but let's not leave it that way for long.

[antgamdia]

Tested it against #3377 and seems to work :)

The test suite (with the mocked plugin, thx Michael!) could be improved to test more cases, but I'd rather like to do it in a follow-up PR and start using the core APIs in the UI as soon as possible to discover more issues or edge cases.

[absoludity]

Thanks Antonio. Looking great with the tests. I'm OK with this landing without pagination or filtering/ordering, but am keen to make sure that the data returned by a test plugin can be specified by the test itself, rather than relying on the hard-coded values set in the code for the test plugin. See inline for details.

[absoludity]

We probably want this to be a _test package? (ie. something that can be imported in tests, but not in non-test code).

[antgamdia]

Good idea. Done ;)

[absoludity]

So with this approach, GetAvailablePackageSummaries and the other methods always return the same data. Another approach is to extend your TestPackagingPlugin struct with:

type TestPackagingPlugin struct {
	packages.UnimplementedPackagesServiceServer
	plugin *plugins.Plugin
	availablePackageSummaries []*corev1.AvailablePackageSummary
	availablePackageDetail *corev1.AvailablePackageDetail
	installedPackageSummaries []*corev1.InstalledPackageSummary
	installedPackageDetail *corev1.InstalledPackageDetail
}

and have each function return whatever data you test has explicitly set the test plugin to use (we can later easily add support for pagination in the test plugin too).

[absoludity]

Why is s.plugin.Name relevant here? Shouldn't PkgVersion just be the semver2 version of the package?

[antgamdia]

It really was for checking the core operation is returning the appropriate PackageAppVersion of the requested plugin, but now we are customizing the responses, we can just add this thing for the required test, and not for every test.

[absoludity]

We really should update the existing pagination to use the number of items for the page offset ASAP. If we keep adding more code that assumes the existing number of pages as the page offset, it's going to get harder to change. Not for this PR, but let's ensure we follow up with that soon.

[antgamdia]

Uff, totally agree... snowball effect :( Added as a note for the iteration discussion.

[absoludity]

Given that currently we're just appending the results from each plugin, we can actually exit this loop if the size of pkgs already exceeds the offset + page size, I think? (We could also do something smart with the page token to avoid re-grabbing results from plugins, but probably not worth it as we'd be better off just implementing the correct pagination.)

[antgamdia]

Good idea, at least we prevent unnecessary plugin requests. Done. But certainly we have to come up with a proper way of pagination :S

[absoludity]

I think we already have one in mind, we just need to update the existing pagination (throughout) to use the number of items as the offset, rather than the number of pages, to then be able to update the next page tokens in the plugins.

[absoludity]

You could add a case here where the plugin will return a 404... and the aggregate will currently return a 500 - whereas we'll want a 404.

[absoludity]

Fine either way, but I've started defining this option once for the set of tests (could even be for the file) and just using it in tests. For eg, see: https://github.com/kubeapps/kubeapps/blob/master/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/create_test.go#L98

[antgamdia]

Good point! I just wrote everything quickly to get it ready for review as soon as possible (it was a bit late yesterday :P), and didn't revisit it to check for refactors and improvements. Thanks for pointing it out!

[absoludity]

Same with the NotFound test.

[absoludity]

Ah - that answers an earlier question: you're setting the plugin name in the package version in the other file just so you can verify the correct versions here (ie. coming from the correct plugin). Again, this would be better if the test itself specified the []*corev1.PackageAppVersions for each plugin, then verified the result (ie. all info is here in the test specification).

[absoludity]

Great, thanks Antonio!

[absoludity]

Sorry, my reply had a typo... I had meant write server_test (ie. rather than having it as part of the server package directly as earlier), but it probably belongs best as plugin_test? Anyway, the key point is that the module ends in _test, but it shouldn't just be _test.

[antgamdia]

Ah, got it! I assumed it was a sort of convention for mocks :D, no problem, moving to plugin_test instead.

[absoludity]

Up to you, but it might be easier to use if, rather than requiring all this data (and lack of data) when creating the test plugin, if the callsite can just create it without any data, then set what they want through public fields.

[absoludity]

Nice.

[absoludity]

You could just replace the above 4 lines with:

contextMsg = fmt.Sprintf("(cluster=[%s], namespace=[%s])", request.GetContext().GetCluster(), request.GetContext().GetNamespace())

if you're OK with "(cluster=[], namespace=[])" when they're not defined.

[antgamdia]

Great, this also applies to the rest of the helm/kapp already implemented operations. I'll send an additional PR for that.

[absoludity]

When we do the pagination correctly, we'll need different sortiing here anyway (since we'll need to keep pulling from each plugin and appending the next item in order. Sounds fun :)

[absoludity]

Fine as is, but really no need for opt1? (you can just pass in ignoreUnexportedOpts below).

[antgamdia]

Oops, just a leftover, thanks!

[absoludity]

Copy-n-paste error? I think this should be "it should fail when calling the core GetAvailablePackageDetail operation when the package is not present" or something?

[antgamdia]

Renamed to (...) when the package is not present in a plugin. However, what I really wanted to highlight with this name is that, currently, the core APIs are failing if one (or more) plugin(s) fails.

If, in the future, we add a fail-safe mode (eg. ignoring failing plugins or letting users configure the strict: true/false when enabling plugins), we might want to revisit these tests.

[absoludity]

So this is fine as is, but just for a bit of explanation of what I'd meant in the previous review: If it's easy to configure the test plugin for each test, then each test above could instead be very explicit rather than referring to pre-built objects defined elsewhere in the file. I'll try to make some time to have a play later in the week perhaps and show you what I mean.

[antgamdia]

I think I'm getting now what you're suggesting. Whereas I do agree on being as explicit as possible in each test case, for a low-specificity test (I mean, just aimed at testing it works), I consider using pre-built objects a more compact and readable was.
But, on the other hand, it requires reading what the object/fn is really doing... tradeoffs

I guess that building the plugin in each test case with the proper returned values (via SetXXX()) should do the trick. I can perform the changes if you want.

[absoludity]

It was fine as is, but thanks for updating. Note that you didn't need to add setters I don't think? What I was suggesting earlier is just making those fields (of the TestPackagingPluginServer as public fields (ie. capital first letter) so that call-sites can just set them. But fine with setters too. Still +1 :)

[absoludity]

I consider using pre-built objects a more compact and readable was.
But, on the other hand, it requires reading what the object/fn is really doing... tradeoffs

Yes, they are trade-offs. For tests, there is the point you mentioned (that you have to look elsewhere to see what the test input or output actually is), and also that you have to munge data into that one object for specific tests (like you were earlier for the PkgVersion), and that changing that one reference object effects many tests.

The opposite of having every object explicitly declared in each test also has its obvious downsides. I usually try for a good test object factory (simple make functions if i've nothing at hand), that enable defining just what is required to be read for the test with good defaults for the rest. But not always easy to get that balance.