Adding filter options to chart-repo #658

gregsidelinger · 2020-02-11T03:11:47Z

Signed-off-by: Greg Sidelinger gate@ilive4code.net

Here are my basic filter changes mentioned in #657

prydonius

Thanks @gregsidelinger, I like the idea! cc @andresmgot @absoludity

prydonius · 2020-02-11T17:59:40Z

cmd/chart-repo/utils_test.go

+	filter := new(filters)
+	filter.Names = append(filter.Names, "wordpress")
+	filter.Names = append(filter.Names, "not-found")
+    charts = chartsFromIndex(index, r, filter)


can you run gofmt?

prydonius · 2020-02-11T18:01:00Z

cmd/chart-repo/utils_test.go

 	assert.Equal(t, len(charts), 2, "number of charts")
+	// filter on name


could we split this up into sub-tests using t.Run? (e.g.

monocular/cmd/chartsvc/handler_test.go

Line 73 in 4043fe3

t.Run(tt.name, func(t *testing.T) {

, but without the for-loop)

I broke them out into separate tests but did not use a loop using r.Run

prydonius · 2020-02-11T18:04:42Z

cmd/chart-repo/utils_test.go

+	filter.Annotations["sync"] = "true"
+	filter.Annotations["not-found"] = "missing"
+	filter.Annotations["no-value"] = ""
+    assert.Equal(t, len(charts), 1, "number of charts")


what chart does this test pick up? I can't see any annotations in https://github.com/helm/monocular/blob/master/cmd/chart-repo/testdata/valid-index.yaml?

My bad, I was doing some cleanup and the updated test yam l did got get committed. It will be there in the next push.

absoludity · 2020-02-11T22:52:11Z

Thanks @gregsidelinger, I like the idea! cc @andresmgot @absoludity

Nice. Yes storing extra data for charts when they are synced, and being able to query based on that data is also something we're needing in Kubeapps (though we're now using a different codebase for sync which supports postgresql as a backend).

prydonius · 2020-02-12T18:49:54Z

cmd/chart-repo/utils.go

+				if v, ok := entry[0].Annotations[a]; ok {
+					if len(av) == 0 {
+						charts = append(charts, newChart(entry, r))
+						continue ENTRYLOOP


I think we can avoid this and improve the readability here by splitting out these filters into separate functions. Also, there might be a bug here. What if I have a name filter and an annotation filter, only the annotation filter will hold?

Something like:

if !matchesAnnotationsFilter(filter.Annotations, entry) || !matchesNamesFilter(filter.Names, entry) { // log skipping chart continue } charts = append(charts, newChart(entry, r))

Those functions can return true if filter.Annotations or filter.Names is empty.

WDYT?

Done, I was thinking the same thing when using the ENTRYLOOP label but was being lazy. Since my intention was to sync if any of the filters matched I did a single function that returns true if a filter matched.

prydonius · 2020-02-12T18:50:56Z

cmd/chart-repo/utils_test.go

+	filter.Annotations["sync-by-name-only"] = ""
+	charts := chartsFromIndex(index, r, filter)
+	assert.Equal(t, len(charts), 1, "number of charts")
+}


let's add a test for both an annotation and name filter

prydonius · 2020-02-12T18:53:43Z

cmd/chart-repo/utils_test.go

+	filter := new(filters)
+	filter.Annotations = make(map[string]string)
+	filter.Annotations["sync"] = "true"
+	filter.Annotations["not-found"] = "missing"


Is the expectation here that it only needs to match one of the annotations and not all of the annotations? I would have assumed that all the annotations need to match for the entry to be selected (I believe this is also how kubectl label filters work).

Good point I did not think about that. My intend was if any of them matched it synced. That is actually how I just rewrote the code as a function.

// return true if entry matches any filter func filterEntry( entry *helmrepo.ChartVersion, filter *filters) bool {

prydonius · 2020-02-12T18:53:57Z

cmd/chart-repo/utils_test.go

+	filter.Annotations = make(map[string]string)
+	filter.Annotations["sync"] = "true"
+	filter.Annotations["not-found"] = "missing"
+	filter.Annotations["sync-by-name-only"] = ""


Is this not testing the same thing as Test_chartsFromIndexFilterByAnnotationDuplicateMatches?

Yes, filter.Annotations["sync-by-name-only"] = "" should not be there, too much copy and pasting.

prydonius · 2020-02-13T19:01:35Z

cmd/chart-repo/utils.go

+		if len(filter.Annotations) > 0 ||
+			len(filter.Names) > 0 {
+			if filterEntry(entry[0], filter){
+				charts = append(charts, newChart(entry, r))


nitpick: it might be simpler to flip this condition and continue if it enters it, and just fallback to the append on line 291 if it matches the filter. i.e. similar to the deprecation condition above

Done. I ended up adding more test data to valid-index.yaml as I removed the ! from if !filterEntry(entry[0], filter) to make the test fail and realized with only two charts of data the test would pass no mater what when filtering.

prydonius · 2020-02-13T19:02:11Z

cmd/chart-repo/utils.go

+			if filterEntry(entry[0], filter){
+				charts = append(charts, newChart(entry, r))
+			}
+			log.WithFields(log.Fields{"name": entry[0].GetName()}).Info("skipping chart as filters did not match")


This would need to be in an else block?

cmd/chart-repo/chart_repo.go

Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Add filter on annoation name only Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Updating filter tests Signed-off-by: Greg Sidelinger <sidelig@nationwide.com> Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Co-Authored-By: Adnan Abdulhussein <adnan@prydoni.us> Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Adding more test data Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

prydonius

sorry for the delay - I was on holiday.

This looks great, can you just run gofmt one more time to ensure it's all properly formatted?

gregsidelinger · 2020-03-04T18:48:34Z

gofmt looks happy on my end.

prydonius · 2020-03-04T19:57:09Z

Thanks!

prydonius · 2020-03-04T19:57:32Z

@andresmgot do you want to create a new Monocular release with this change?

andresmgot · 2020-03-05T09:40:21Z

sure, thanks for the review!

https://github.com/helm/monocular/releases/tag/v1.10.0

helm-bot added the size/M label Feb 11, 2020

prydonius reviewed Feb 11, 2020

View reviewed changes

helm-bot added size/L and removed size/M labels Feb 12, 2020

prydonius reviewed Feb 12, 2020

View reviewed changes

prydonius reviewed Feb 13, 2020

View reviewed changes

gregsidelinger and others added 6 commits February 13, 2020 20:55

Adding filter options to chart-repo

05a2434

Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Updating filter tests

7ae1031

Add filter on annoation name only Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Moving filter logic to function

57ce3c5

Updating filter tests Signed-off-by: Greg Sidelinger <sidelig@nationwide.com> Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Update cmd/chart-repo/chart_repo.go

b7bcef0

Co-Authored-By: Adnan Abdulhussein <adnan@prydoni.us> Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Update cmd/chart-repo/chart_repo.go

9a445e2

Co-Authored-By: Adnan Abdulhussein <adnan@prydoni.us> Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

Minor filter updates

c95b4f0

Adding more test data Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

gregsidelinger force-pushed the filter branch from b608901 to c95b4f0 Compare February 14, 2020 01:55

Adding glob matching to name filters

8f1f782

Signed-off-by: Greg Sidelinger <gate@ilive4code.net>

prydonius approved these changes Mar 3, 2020

View reviewed changes

prydonius merged commit bc0c3d4 into helm:master Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding filter options to chart-repo #658

Adding filter options to chart-repo #658

gregsidelinger commented Feb 11, 2020

prydonius left a comment •

edited

Loading

prydonius Feb 11, 2020

gregsidelinger Feb 12, 2020

prydonius Feb 11, 2020

gregsidelinger Feb 12, 2020

prydonius Feb 11, 2020

gregsidelinger Feb 12, 2020

absoludity commented Feb 11, 2020

prydonius Feb 12, 2020

gregsidelinger Feb 13, 2020

prydonius Feb 12, 2020

gregsidelinger Feb 13, 2020

prydonius Feb 12, 2020

gregsidelinger Feb 13, 2020 •

edited

Loading

prydonius Feb 12, 2020

gregsidelinger Feb 13, 2020

prydonius Feb 13, 2020

gregsidelinger Feb 14, 2020

prydonius Feb 13, 2020

prydonius left a comment

gregsidelinger commented Mar 4, 2020

prydonius commented Mar 4, 2020

prydonius commented Mar 4, 2020

andresmgot commented Mar 5, 2020 •

edited

Loading

		assert.Equal(t, len(charts), 2, "number of charts")
		// filter on name

Adding filter options to chart-repo #658

Adding filter options to chart-repo #658

Conversation

gregsidelinger commented Feb 11, 2020

prydonius left a comment • edited Loading

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 commented Feb 11, 2020

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

gregsidelinger Feb 13, 2020 • edited Loading

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

prydonius left a comment

Choose a reason for hiding this comment

gregsidelinger commented Mar 4, 2020

prydonius commented Mar 4, 2020

prydonius commented Mar 4, 2020

andresmgot commented Mar 5, 2020 • edited Loading

prydonius left a comment •

edited

Loading

gregsidelinger Feb 13, 2020 •

edited

Loading

andresmgot commented Mar 5, 2020 •

edited

Loading