Move code from asset syncer to pkg #2976

gfichtenholt · 2021-06-13T08:27:24Z

The main goal of this PR is to address one feedback from the last PR I pushed #2915, where Michael asked:
"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?"
So this PR is addressing this by pulling out some (private) code from asset-syncer into a pkg directory where it can be used by both asset-syncer and the fluxv2 plug in, namely:

tar utils
http-client
helm/index
In the future, as proceed with the implementation of fluxv2 plug-in, I expect to pull out even more code out of asset-syncer into these or new modules.
Another small change is some code cleanup and an addition of a unit test

antgamdia

Thank you for splitting up the code! Now the fluxv2 code looks simpler :)

antgamdia · 2021-06-14T10:20:55Z

pkg/tarutil/tarutil.go

-	log.Infof("+getHelmIndexFileFromURL(%s) 1", indexURL)
-	// Get the response bytes from the url
-	response, err := http.Get(indexURL)
+func FetchDetailFromTarball(name string, chartTarballURL string, userAgent string, authz string, netClient httpclient.Client) (map[string]string, error) {


I'd rename this fn to sth like FetchChartDetailFromTarball (to highlight it's just for chart tarballs, not any tarball) since it is now publicly available under the tarutil package

Idem here about documenting

will do. Thanks

antgamdia · 2021-06-14T10:23:30Z

pkg/helm/testdata/valid-index.yaml

@@ -0,0 +1,69 @@
+entries:


Question: are we using files for testing elsewehere? Probably it will make sense due to the nature of the test, but I'm not 100% sure if it's the way to go.

yes, this file is also under asset syncer testdata and is used there for a bunch of tests to test asset syncer-specific features that only exist there. If/when I end up moving more code out of asset syncer into pkg utils there maybe a point where there no longer any tests left there using this file, at which point I will delete it. But not yet

Yep, until recently it was necessary to create test files like this and ensure your code accepts a director so you can point it to your test data during tests. More recently go added the fstest package (https://golang.org/pkg/testing/fstest/) which I've used in cmd/kubeapps-apis/server/plugins_test.go . But agree, not worth changing this at all, as part of the benefit of this actual file test data is that it's an exact copy of a subset of a real index.yaml (rather than something we construct in memory to match).

antgamdia · 2021-06-14T10:31:40Z

pkg/helm/index.go

+	return c
+}
+
+func ChartsFromIndex(contents []byte, r *models.Repo, shallow bool) ([]models.Chart, error) {


In exported (at least) functions I'd like to have some documentation around it. Perhaps this PR is just about moving the existing code, but since it's now public, I think it might be worthwhile to add a couple of lines, no?

https://blog.golang.org/godoc

sure will do

antgamdia · 2021-06-14T10:32:44Z

pkg/http-client/httpclient.go

+	Do(req *http.Request) (*http.Response, error)
+}
+
+func Get(url string, cli Client, headers map[string]string) ([]byte, error) {


Idem here about documenting

antgamdia · 2021-06-14T10:33:09Z

pkg/tarutil/test/utils.go

+	Name, Body string
+}
+
+func CreateTestTarball(w io.Writer, files []TarballFile) {


Idem here about documenting

absoludity

Thanks Greg. A few comments in line, see what you think but +1 either way (assuming it's not changing current functionality for the existing helm support - wasn't 100% with part of the refactor where you mention just returning the latest entry).

absoludity · 2021-06-15T01:12:02Z

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

@@ -214,14 +232,16 @@ func (s *Server) GetPackageMeta(ctx context.Context, request *corev1.GetAvailabl
 	log.Infof("Found chart url: [%s]", *url)

 	// unzip and untar .tgz file
-	meta, err := fetchMetaFromChartTarball(request.AvailablePackageRef.Identifier, *url)
+	// TODO: userAgent, authz and netClient w/TLS config similar to asset-syncer utils.initNetClient(),


If you can add your gh nick to the todo (ie. // TODO (gfichtenholz): ...) it'd be helpful (as the obvious committer from git is a pain to dig for as soon as this is refactored and copied elsewhere). Same for other TODOs in this change.

absoludity · 2021-06-15T01:14:53Z

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

@@ -238,14 +258,21 @@ func (s *Server) pullChartTarball(ctx context.Context, packageRef *corev1.Availa
 		Resource: fluxHelmCharts,
 	}

+	// TODO: use the namespace from the request


Is this a TODO only because it's a pain to construct a request currently (as we're not yet mapping the namespace to the URL)? Otherwise why not just use request.Namespace or whatever the field is?

Nothing hard about it. Just didn't get to it yet :-) Will happen soon

absoludity · 2021-06-15T01:16:24Z

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

+	// @gfichtenholt 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


Personally don't think we need the history of the discussion there, I think it's enough to just state the current state - something along the lines of the ListOptions FieldSelector not supporting the fields required (and link to the relevant issue/pr comment for more context, if it's helpful).

ok you got it

absoludity · 2021-06-15T01:16:57Z

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

+// 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.


Same here. For example, state the TODO and link to more context (the gh issue comment).

absoludity · 2021-06-15T01:27:07Z

pkg/helm/index.go

+//
+// given a an array of bytes containg the contents of index.yaml from a helm repo it will return
+// all Chart models. shallow flag controls whether only the latest version of the charts is kept
+// or all versions
+//


See the link Antonio provided... it's fine as is, but the normal convention would be for something like

Suggested change

//

// given a an array of bytes containg the contents of index.yaml from a helm repo it will return

// all Chart models. shallow flag controls whether only the latest version of the charts is kept

// or all versions

//

// ChartsFromIndex receives an array of bytes containing the contents of index.yaml from a helm repo and returns

// all Chart models from that index. The shallow flag controls whether only the latest version of the charts is returned

// or all versions.

np, will make that change. Thanks

absoludity · 2021-06-15T01:31:51Z

pkg/helm/index.go

+		// "mariadb" version "9.3.11", "9.3.10", etc. For entry "mariadb", bitnami catalog has
+		// almost 200 chart versions going all the way back many years to version "2.1.4".
+		// So for now, let's just keep track of the latest, not to overwhelm the caller with
+		// all these outdated versions


This comment doesn't appear to be related to the GetDeprecated below, but perhaps the following expression appending the new chart? But even there, newChart receives a helm.ChartVersions so not sure which you correctly pass, so unsure where you're using entry[0] instead, or whether you can do so here without breaking the current helm support (which expects all versions).

disagree on this one. Comment is right after the line
for _, entry := range index.Entries { . and really applies to the entire block on code, including the if statement if entry[0].GetDeprecated() {I immediately following it. I don't believe I broke anything, except possibly the format of the log message. The rest of the code should be exactly as it is today in asset syncer. https://github.com/kubeapps/kubeapps/blob/master/cmd/asset-syncer/utils.go#L801

Great. Yes, I was looking at the following line (the one at the same indent), not the previous. The last sentence of the comment sounded as though you'd changed something, but I couldn't identify the change. +1

absoludity · 2021-06-15T01:32:29Z

pkg/helm/index.go

+	return &index, nil
+}
+
+// Takes an entry from the index and constructs a database representation of the


Suggested change

// Takes an entry from the index and constructs a database representation of the

// Takes an entry from the index and constructs a model representation of the

absoludity · 2021-06-15T01:39:50Z

pkg/helm/testdata/valid-index.yaml

@@ -0,0 +1,69 @@
+entries:


Yep, until recently it was necessary to create test files like this and ensure your code accepts a director so you can point it to your test data during tests. More recently go added the fstest package (https://golang.org/pkg/testing/fstest/) which I've used in cmd/kubeapps-apis/server/plugins_test.go . But agree, not worth changing this at all, as part of the benefit of this actual file test data is that it's an exact copy of a subset of a real index.yaml (rather than something we construct in memory to match).

gfichtenholt added 8 commits June 9, 2021 23:21

step 1

5c6082a

Merge branch 'master' into tbd

454b4ed

step 2

770dee8

Merge branch 'master' into tbd

3a1dc2a

step 3

b52944d

Merge branch 'master' into tbd

3fca6e4

step 4

9652fe7

step 5

9a62657

gfichtenholt added this to In progress in Kubeapps via automation Jun 13, 2021

gfichtenholt self-assigned this Jun 13, 2021

gfichtenholt requested a review from absoludity June 13, 2021 10:36

antgamdia reviewed Jun 14, 2021

View reviewed changes

incorporate Antonio's feedback

c3bb92d

absoludity approved these changes Jun 15, 2021

View reviewed changes

incorporated Michael's feedback

2cb169e

gfichtenholt merged commit 11c64cf into vmware-tanzu:master Jun 15, 2021

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

gfichtenholt deleted the move-code-from-asset-syncer-to-pkg branch June 23, 2021 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move code from asset syncer to pkg #2976

Move code from asset syncer to pkg #2976

gfichtenholt commented Jun 13, 2021

antgamdia left a comment •

edited

antgamdia Jun 14, 2021

antgamdia Jun 14, 2021

gfichtenholt Jun 14, 2021

antgamdia Jun 14, 2021

gfichtenholt Jun 14, 2021

absoludity Jun 15, 2021

antgamdia Jun 14, 2021

gfichtenholt Jun 14, 2021

antgamdia Jun 14, 2021

gfichtenholt Jun 14, 2021

antgamdia Jun 14, 2021

gfichtenholt Jun 14, 2021

absoludity left a comment

absoludity Jun 15, 2021

gfichtenholt Jun 15, 2021

absoludity Jun 15, 2021

gfichtenholt Jun 15, 2021

absoludity Jun 15, 2021

gfichtenholt Jun 15, 2021

absoludity Jun 15, 2021

gfichtenholt Jun 15, 2021

absoludity Jun 15, 2021

gfichtenholt Jun 15, 2021

absoludity Jun 15, 2021

gfichtenholt Jun 15, 2021

absoludity Jun 15, 2021

absoludity Jun 15, 2021

gfichtenholt Jun 15, 2021

absoludity Jun 15, 2021

	// Takes an entry from the index and constructs a database representation of the
	// Takes an entry from the index and constructs a model representation of the

Move code from asset syncer to pkg #2976

Move code from asset syncer to pkg #2976

Conversation

gfichtenholt commented Jun 13, 2021

antgamdia left a comment • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgamdia left a comment •

edited