Get package details prototype #2915

gfichtenholt · 2021-05-31T08:51:54Z

Description of the change

A prototype for fluxv2 plugin's feature that returns package details

Benefits

Possible drawbacks

Applicable issues

[kubeapps-apis] Investigate provision of chart Readme and values #2864

Additional information

absoludity · 2021-06-01T01:45:09Z

cmd/kubeapps-apis/proto/kubeappsapis/core/packages/v1alpha1/packages.proto

+
+message GetPackageMetaResponse {
+  message PackageMeta {
+    string readme = 1;


I was thinking about this field - Helm charts have a readme, but a more general option might be longDescription, noting that it can be markdown, but we can discuss that in the design doc soon.

Michael, this is not ready for review in any shape. Its a draft just to see what it costs to make the "electrons flow". When I am ready for review the interface will be much more generic and not look like that. So no need to worry

Ah sorry... I thought you'd created a draft PR because it wasn't ready for review but wanted some feedback (something I often do). No probs, I'll not look at something until you request it :)

absoludity · 2021-06-01T01:54:39Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

+
+	log.Infof("created chart: [%v]", newChart)
+
+	return nil, status.Errorf(codes.Unimplemented, "not implemented yet")


Guessing you plan at this point to wait for the chart to reconcile before returning the metadata, though I don't see it on the CR (https://fluxcd.io/docs/components/source/helmcharts/#specification) so we'd need to parse the chart itself, right? (which is cached by fluxv2?)

please hold comments until I mark it ready for review :-)

gfichtenholt · 2021-06-08T00:55:13Z

I marked it ready for review. To be clear on what this is - just a prototype piece of
code that shows a few things like creating a HelmChart CRD, waiting until its fully ready, unpacking the helm chart etc, i.e. the general direction in which I will implement GetAvailablePackageDetail() API
I wanted to push this code before I start a re-factor to conform to the latest design spec
because in addition to the above, it also has some general cleanup/improvements and I wanted
to make sure those are not lost. So don't worry about the API name, etc - that will all be
changed to conform to spec as soon as we finalize it

absoludity

Nice work Greg. I'm OK if you want to land this as is, slight preference for at least using the endpoint name that we're going to move to, but you may have reasons to prefer to land and continue. A few comments in line.

absoludity · 2021-06-08T02:21:23Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

+)
+
+// these should really be constants, rather than global vars
+// but alas, go does not allow const structs (why???)


Because an instance of a struct is not a compile-time object, therefore, not really constant. I assume the reason for that is for the speed of the compiler (it does not need to check whether code is updating things that are declared as constants).

I'd personally prefer not to set a precedent of using global vars at all, and instead just instantiate the GroupVersionResource using the const strings that you already have.

ok, sounds good. I'll change this.
FWIW, there was a lot of debate on the subject of const structs in go. What I got from it, it is technically possible just requires a lot of work, and there wasn't enough "bang for the buck" (literally an expression someone at google used) to make it happen. bleh

absoludity · 2021-06-08T02:23:16Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

@@ -172,6 +201,130 @@ func (s *Server) GetAvailablePackages(ctx context.Context, request *corev1.GetAv
 	}, nil
 }

+// GetPackageMeta streams the package metadata based on the request.
+func (s *Server) GetPackageMeta(ctx context.Context, request *corev1.GetPackageMetaRequest) (*corev1.GetPackageMetaResponse, error) {


I think this is what you were referring to, that I should ignore? As in, this will later be changed to GetAvailablePackageDetail or something? If so, why not just add it as we plan it to be (just this one call)?

yeah, this will happen, don't worry about that

absoludity · 2021-06-08T02:25:51Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

+		return nil, err
+	}
+
+	resourceIfc := client.Resource(chartsResource).Namespace("default")


Why is the namespace hard-coded here? Fine if it's just for demoning right now.

just a placeholder, like I said, a lot of refactoring to be done

absoludity · 2021-06-08T02:31:10Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

+		return nil, err
+	}
+
+	// TODO it'd be better if we could filter on server-side


You should be able to use the metav1.ListOptions{} above to specify filtering using the FieldSelector. More info at https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/ . You may even be able to specify that the pull should be complete to be included (ie. that status conditions ready is true), not sure, but that'd be nice.

Again, at this point it's fine if you just want to update your comment to link to that and do it in a later PR.

As I remember I did try a few things here. The problem is the set of supported fields in FieldSelector is very small. things like spec.chart are certainly not supported. see kubernetes/client-go#713 and https://github.com/flant/shell-operator/blob/8fa3c3b8cfeb1ddb37b070b7a871561fdffe788b/HOOKS.md#fieldselector. Nevertheless, I made a TODO comment here just as you suggested to see if I can improve later

absoludity · 2021-06-08T02:40:36Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

+}
+
+func waitUntilChartPullComplete(watcher watch.Interface) (*string, error) {
+	ch := watcher.ResultChan()


Nice - great use of both channel and the watcher to handle the async request. Note that this effectively makes pullChartTarball a blocking call, which is fine in it's current use (where it's only called in a context of one chart). Not for now, but in the future you might instead want to consider something like passing a results channel (of string urls) to pullChartTarball, so it returns immediately and you wait on the results channel at the call-site, which would mean you could call it for 20 different charts and just wait for the results to come in whatever order they happen to take, rather than serially.

absoludity · 2021-06-08T02:44:22Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

+//
+// TODO the semantics of this really should be do we need to keep polling or not
+//
+func isChartPullComplete(unstructuredChart *unstructured.Unstructured) (bool, error) {


As above, hopefully this fn isn't required if we can only list charts that we know are ready.

absoludity · 2021-06-08T02:46:00Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/utils.go

@@ -0,0 +1,134 @@
+/*


Is the functionality in this utils file already available in the existing codebase? If so, I'd be keen to import and re-use that?

yeah, kind of, but its private to asset-syncer. now. Needs to be re-packaged for re-used. I will put a TODO item to see if I can clean it up

absoludity · 2021-06-08T02:47:28Z

cmd/kubeapps-apis/proto/kubeappsapis/core/packages/v1alpha1/packages.proto

@@ -22,6 +22,11 @@ service PackagesService {
      get: "/core/packages/v1alpha1/packagerepositories"
    };
  }
+  rpc GetPackageMeta(kubeappsapis.core.packages.v1alpha1.GetPackageMetaRequest) returns (kubeappsapis.core.packages.v1alpha1.GetPackageMetaResponse) {


Can we just use GetInstalledPackageDetails? Fine if not, we'll be updating it soon.

one thing at a time :-). It will probably be in my next PR

gfichtenholt added 3 commits May 31, 2021 01:35

step 1

6cdff5e

step 2

4cdfddc

step 3

fc7bfe9

gfichtenholt linked an issue May 31, 2021 that may be closed by this pull request

[kubeapps-apis] Investigate provision of chart Readme and values #2864

Closed

gfichtenholt added this to In progress in Kubeapps via automation May 31, 2021

gfichtenholt self-assigned this May 31, 2021

absoludity reviewed Jun 1, 2021

View reviewed changes

gfichtenholt added 6 commits May 31, 2021 19:00

step 4

d0f3087

Merge branch 'master' into get-package-meta-prototype

8cd9352

step 5

a03853e

step 6

6afc95c

step 7

b8d9991

Merge branch 'master' into get-package-meta-prototype

2bd5b53

gfichtenholt changed the title ~~Get package meta prototype~~ Get package details prototype Jun 8, 2021

gfichtenholt marked this pull request as ready for review June 8, 2021 00:54

absoludity approved these changes Jun 8, 2021

View reviewed changes

step 8

53a58b6

gfichtenholt merged commit a6d2a17 into vmware-tanzu:master Jun 8, 2021

Kubeapps automation moved this from In progress to Done Jun 8, 2021

gfichtenholt mentioned this pull request Jun 13, 2021

Move code from asset syncer to pkg #2976

Merged

gfichtenholt deleted the get-package-meta-prototype branch June 23, 2021 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get package details prototype #2915

Get package details prototype #2915

gfichtenholt commented May 31, 2021 •

edited

absoludity Jun 1, 2021

gfichtenholt Jun 1, 2021

absoludity Jun 1, 2021

absoludity Jun 1, 2021

gfichtenholt Jun 1, 2021

gfichtenholt commented Jun 8, 2021

absoludity left a comment

absoludity Jun 8, 2021

gfichtenholt Jun 8, 2021

absoludity Jun 8, 2021

gfichtenholt Jun 8, 2021

absoludity Jun 8, 2021

gfichtenholt Jun 8, 2021

absoludity Jun 8, 2021

gfichtenholt Jun 8, 2021

absoludity Jun 8, 2021

absoludity Jun 8, 2021

absoludity Jun 8, 2021

gfichtenholt Jun 8, 2021

absoludity Jun 8, 2021

gfichtenholt Jun 8, 2021


		log.Infof("created chart: [%v]", newChart)

		return nil, status.Errorf(codes.Unimplemented, "not implemented yet")

Get package details prototype #2915

Get package details prototype #2915

Conversation

gfichtenholt commented May 31, 2021 • edited

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfichtenholt commented Jun 8, 2021

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfichtenholt commented May 31, 2021 •

edited