feature: support images filter flag #2410

ZYecho · 2018-11-02T02:27:48Z

Signed-off-by: zhangyue [email protected]

Ⅰ. Describe what this PR did

add filter flag support for pouch images, support three filters: since/before/reference

before ([:], or image@digest) - filter images created before given id or references
since ([:], or image@digest) - filter images created since given id or references
The reference filter shows only images whose reference matches the specified pattern. may have multi reference filter. the relationship between multi reference filter is or.

Ⅱ. Does this pull request fix one issue?

Related to #2405
fix #2371

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

add api testcase and cli testcase.

Ⅳ. Describe how to verify it

pouch images

IMAGE ID       IMAGE NAME                                          SIZE
8c811b4aec35   registry.hub.docker.com/library/busybox:1.28        710.81 KB
4ab4c602aa5e   registry.hub.docker.com/library/hello-world:linux   5.25 KB

pouch images -f reference=registry.hub.docker.com/library/busybox:*

IMAGE ID       IMAGE NAME                                     SIZE
8c811b4aec35   registry.hub.docker.com/library/busybox:1.28   710.81 KB

pouch images -f after=registry.hub.docker.com/library/busybox:1.28
Got
Error: failed to get image list: {"message":"Invalid filter after"}

pouch images -f before=registry.hub.docker.com/library/hello-world:linux

IMAGE ID       IMAGE NAME                                     SIZE
8c811b4aec35   registry.hub.docker.com/library/busybox:1.28   710.81 KB

Ⅴ. Special notes for reviews

Use the same filter package as pouch events

codecov · 2018-11-02T02:27:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@401e132). Click here to learn what that means.
The diff coverage is 70%.

@@            Coverage Diff            @@
##             master    #2410   +/-   ##
=========================================
  Coverage          ?   68.91%           
=========================================
  Files             ?      276           
  Lines             ?    18287           
  Branches          ?        0           
=========================================
  Hits              ?    12602           
  Misses            ?     4255           
  Partials          ?     1430

Flag	Coverage Δ
#criv1alpha1test	`31.57% <16.25%> (?)`
#criv1alpha2test	`35.62% <16.25%> (?)`
#integrationtest	`40.33% <58.75%> (?)`
#nodee2etest	`32.98% <16.25%> (?)`
#unittest	`26.59% <15%> (?)`

Impacted Files	Coverage Δ
cri/v1alpha1/cri.go	`60.59% <ø> (ø)`
daemon/mgr/system.go	`73.28% <ø> (ø)`
cri/v1alpha2/cri.go	`69.65% <ø> (ø)`
client/image_list.go	`64.28% <37.5%> (ø)`
apis/server/image_bridge.go	`74.1% <40%> (ø)`
daemon/mgr/image.go	`59.63% <71.42%> (ø)`
apis/filters/parse.go	`80.72% <88.88%> (ø)`

allencloud · 2018-11-02T02:35:55Z

CI fails with code check. Please take a look. @ZYecho Thanks.

ZYecho · 2018-11-02T02:52:04Z

CI fails with code check. Please take a look. @ZYecho Thanks.

@allencloud fixed it, also cc @Ace-Tang @fuweid request for review.

HusterWan · 2018-11-02T05:49:44Z

apis/filters/parse.go

@@ -150,3 +157,33 @@ func FromParam(p string) (Args, error) {
 	}
 	return args, nil
 }
+
+//FromFilterOpts parse key=value to Args string from cli opts


Maybe we can add some test cases for this new function, WDYT ? @ZYecho

good idea, I add a testcase to cover this.

could we add space between // and FromFilterOpts?

daemon/mgr/image.go

fuweid · 2018-11-05T01:43:46Z

apis/filters/parse.go

+func (args Args) Validate(accepted map[string]bool) error {
+	for name := range args.fields {
+		if !accepted[name] {
+			return errors.New("Invalid filter " + name)


could we avoid to start with a capital letter in the error string? The reason is here https://github.com/golang/go/wiki/Errors#errors

could you add accepted filter name ? make user more clearly

could you add accepted filter name ? make user more clearly

Is there a need to do here? I find that other validate func in pouch also not do this, maybe We can do this by adding docs or cli usage? wdyt? @Ace-Tang

apis/filters/parse_test.go

fuweid · 2018-11-05T01:52:05Z

apis/filters/parse.go

@@ -150,3 +157,33 @@ func FromParam(p string) (Args, error) {
 	}
 	return args, nil
 }
+
+//FromFilterOpts parse key=value to Args string from cli opts


could we add space between // and FromFilterOpts?

fuweid · 2018-11-05T01:52:19Z

apis/filters/parse.go

+	return nil
+}
+
+//FamiliarMatch decide the ref match the pattern or not


could we add space between // and FromFilterOpts?

daemon/mgr/image.go

fuweid · 2018-11-05T01:59:53Z

daemon/mgr/image.go

+	beforeImages := filter.Get("before")
+	if len(beforeImages) > 0 {
+		// use the last one if multi before images was assigned
+		beforeFilter, err = mgr.GetImage(ctx, beforeImages[len(beforeImages)-1])


how can we make the beforeImages is sorted?

To be honest, I'm curious about the logic dealing with multi-since or multi-before, I think we can not guess the user want to the first one or the last one, use the last input or throw err if multi-since or multi-before? wdyt? @fuweid

so, could we limit the multi-since/multi-before here? Since there is no reason for this requirement.

@fuweid I test the same scenario in moby, have the same problem. I think the single-since/since-before is reasonable, while multi is undefined. So in here, I want to throw err if len > 1, wdyt?

fuweid · 2018-11-05T02:02:20Z

daemon/mgr/image.go

+		if filter.Contains("reference") {
+			var found bool
+			for _, ref := range mgr.localStore.GetReferences(img.ID) {
+				for _, pattern := range filter.Get("reference") {


could we retrieve filter.Get("reference") before? I think the Get will allocate new memory each loop.

seems much condition here

what is the condition?

Ace-Tang · 2018-11-05T02:14:40Z

apis/filters/parse.go

+func (args Args) Validate(accepted map[string]bool) error {
+	for name := range args.fields {
+		if !accepted[name] {
+			return errors.New("Invalid filter " + name)


could you add accepted filter name ? make user more clearly

apis/server/image_bridge.go

cli/image_list.go

Ace-Tang · 2018-11-05T02:18:19Z

daemon/mgr/image.go

+		if filter.Contains("reference") {
+			var found bool
+			for _, ref := range mgr.localStore.GetReferences(img.ID) {
+				for _, pattern := range filter.Get("reference") {


seems much condition here

fuweid · 2018-11-08T16:08:20Z

test/cli_images_test.go

+
+	//Test invalid filter
+	invalidRes := command.PouchRun("images", "-f", "after="+busyboxImage)
+	c.Assert(invalidRes.Stderr(), check.Equals, "Error: failed to get image list: {\"message\":\"Invalid filter after\"}\n\n")


the case is failed

/home/travis/gopath/src/github.com/alibaba/pouch/test/cli_images_test.go:114: c.Assert(invalidRes.Stderr(), check.Equals, "Error: failed to get image list: {\"message\":\"Invalid filter after\"}\n\n") ... obtained string = "" + ... "Error: failed to get image list: {\"message\":\"invalid filter after\"}\n" + ... "\n" ... expected string = "" + ... "Error: failed to get image list: {\"message\":\"Invalid filter after\"}\n" + ... "\n"

I was thinking that we can do this in api level test and only check the status. The message is out of control.
But the status should be stable. WDYT?

agree, update it later.

ZYecho · 2018-11-09T02:08:54Z

@fuweid all mentioned have fixed. PTAL, thanks.

fuweid · 2018-11-09T03:23:41Z

daemon/mgr/image.go

+
+	// refuse undefined behavior
+	if len(beforeImages) > 1 || len(sinceImages) > 1 {
+		return nil, pkgerrors.Wrapf(errtypes.ErrInvalidParam, "can't use since or before filter more than one")


could we split the error into two part? I think it can make the error more clear.

fuweid · 2018-11-09T03:25:22Z

test/api_image_list_test.go

+	c.Assert(reflect.DeepEqual(got[0].RepoTags, []string{repoTag}), check.Equals, true)
+	c.Assert(reflect.DeepEqual(got[0].RepoDigests, []string{repoDigest}), check.Equals, true)
+
+	// check invalid filter


could we split it into other test case function? I think it is too long.

HusterWan · 2018-11-09T03:44:35Z

client/image_list_test.go

@@ -12,14 +12,15 @@ import (

 	"github.com/alibaba/pouch/apis/types"

+	"github.com/alibaba/pouch/apis/filters"


put this line in the second part, thanks @ZYecho

thanks, update.

Signed-off-by: zhangyue <[email protected]>

Ace-Tang · 2018-11-09T03:58:50Z

lgtm

HusterWan · 2018-11-09T06:37:06Z

LGTM, thanks for your works @ZYecho

pouchrobot added kind/feature size/XL labels Nov 2, 2018

ZYecho force-pushed the add-images-filter branch from 9487210 to 29c8c67 Compare November 2, 2018 02:49

HusterWan reviewed Nov 2, 2018

View reviewed changes

daemon/mgr/image.go Show resolved Hide resolved

ZYecho force-pushed the add-images-filter branch 4 times, most recently from f3bc3fe to 39e74c0 Compare November 2, 2018 09:07

fuweid reviewed Nov 5, 2018

View reviewed changes

Ace-Tang reviewed Nov 5, 2018

View reviewed changes

ZYecho force-pushed the add-images-filter branch 2 times, most recently from 2139681 to d9027ce Compare November 5, 2018 06:16

ZYecho mentioned this pull request Nov 6, 2018

support a common filter for container, volume and images choose #2405

Closed

fuweid reviewed Nov 8, 2018

View reviewed changes

ZYecho force-pushed the add-images-filter branch 2 times, most recently from 55b6f30 to a636407 Compare November 9, 2018 02:00

fuweid reviewed Nov 9, 2018

View reviewed changes

ZYecho force-pushed the add-images-filter branch from a636407 to 3411c45 Compare November 9, 2018 03:35

HusterWan reviewed Nov 9, 2018

View reviewed changes

feature: support images filter flag

3248c28

Signed-off-by: zhangyue <[email protected]>

ZYecho force-pushed the add-images-filter branch from 3411c45 to 3248c28 Compare November 9, 2018 03:54

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 9, 2018

HusterWan merged commit 9a98a65 into AliyunContainerService:master Nov 9, 2018

This was referenced Nov 9, 2018

WeeklyReport of PouchContainer from 2018-11-02 to 2018-11-09 #2457

Closed

WeeklyReport of PouchContainer from 2018-11-09 to 2018-11-16 #2471

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: support images filter flag #2410

feature: support images filter flag #2410

ZYecho commented Nov 2, 2018 •

edited

Loading

codecov bot commented Nov 2, 2018 •

edited

Loading

allencloud commented Nov 2, 2018

ZYecho commented Nov 2, 2018

HusterWan Nov 2, 2018

ZYecho Nov 2, 2018

fuweid Nov 5, 2018

ZYecho Nov 5, 2018

fuweid Nov 5, 2018

Ace-Tang Nov 5, 2018

ZYecho Nov 5, 2018

fuweid Nov 5, 2018

fuweid Nov 5, 2018

fuweid Nov 5, 2018

ZYecho Nov 5, 2018 •

edited

Loading

fuweid Nov 6, 2018

ZYecho Nov 6, 2018

fuweid Nov 8, 2018

fuweid Nov 5, 2018

Ace-Tang Nov 5, 2018

fuweid Nov 5, 2018

Ace-Tang Nov 5, 2018

Ace-Tang Nov 5, 2018

fuweid Nov 8, 2018 •

edited

Loading

ZYecho Nov 9, 2018

ZYecho commented Nov 9, 2018

fuweid Nov 9, 2018 •

edited

Loading

ZYecho Nov 9, 2018

fuweid Nov 9, 2018

ZYecho Nov 9, 2018

HusterWan Nov 9, 2018

ZYecho Nov 9, 2018

Ace-Tang commented Nov 9, 2018

HusterWan commented Nov 9, 2018

		@@ -12,14 +12,15 @@ import (

		"github.com/alibaba/pouch/apis/types"

		"github.com/alibaba/pouch/apis/filters"

feature: support images filter flag #2410

feature: support images filter flag #2410

Conversation

ZYecho commented Nov 2, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Nov 2, 2018 • edited Loading

Codecov Report

allencloud commented Nov 2, 2018

ZYecho commented Nov 2, 2018

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

ZYecho Nov 5, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZYecho commented Nov 9, 2018

fuweid Nov 9, 2018 • 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

Ace-Tang commented Nov 9, 2018

HusterWan commented Nov 9, 2018

ZYecho commented Nov 2, 2018 •

edited

Loading

codecov bot commented Nov 2, 2018 •

edited

Loading

ZYecho Nov 5, 2018 •

edited

Loading

fuweid Nov 8, 2018 •

edited

Loading

fuweid Nov 9, 2018 •

edited

Loading