feature: support pouch ps filter #1595

Ace-Tang · 2018-06-25T15:05:17Z

support pouch ps --filter, not only equal condition, but also
unequal condition, like pouch ps --filter label=a!=b

Fixes: #1562.

Signed-off-by: Ace-Tang aceapril@126.com

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes: #1562

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io · 2018-06-26T02:14:53Z

Codecov Report

Merging #1595 into master will increase coverage by 23.62%.
The diff coverage is 63.49%.

@@            Coverage Diff             @@
##           master   #1595       +/-   ##
==========================================
+ Coverage   17.97%   41.6%   +23.62%     
==========================================
  Files         224     273       +49     
  Lines       14780   17699     +2919     
==========================================
+ Hits         2657    7364     +4707     
+ Misses      11950    9423     -2527     
- Partials      173     912      +739

Impacted Files	Coverage Δ
daemon/mgr/container.go	`50.19% <ø> (+50.19%)`	⬆️
daemon/mgr/container_types.go	`76.11% <ø> (+43.28%)`	⬆️
cri/v1alpha1/cri.go	`0% <0%> (ø)`	⬆️
cri/v1alpha2/cri.go	`0% <0%> (ø)`	⬆️
cli/ps.go	`0% <0%> (ø)`	⬆️
apis/server/image_bridge.go	`65.82% <100%> (+54.13%)`	⬆️
client/container_list.go	`68.75% <28.57%> (-31.25%)`	⬇️
daemon/mgr/network.go	`68.14% <40%> (+64.92%)`	⬆️
apis/server/container_bridge.go	`82.01% <60%> (+82.01%)`	⬆️
pkg/utils/filters/filter.go	`67.5% <67.5%> (ø)`
... and 137 more

yyb196 · 2018-06-26T06:37:58Z

daemon/mgr/container_list.go

+				// consider if need to add non-running container
+				match = isNonStop || fc.all
+			}
+			break


one match condition will stop this filter routine. what if people need both -f label=site=daily and -f label=network!=overlay

Modify the logic, jump until condition not equal, and add flag jump to determine if a condition is useful. Test case has add for this case.

Letty5411 · 2018-06-26T08:39:40Z

test/cli_ps_test.go

+	labelA := "equal-label-a"
+	command.PouchRun("run", "-d", "--name", labelA, "-l", "a=b", busyboxImage, "top").Assert(c, icmd.Success)
+	defer DelContainerForceMultyTime(c, labelA)
+	output := command.PouchRun("inspect", labelA).Stdout()


How about use :
output := command.PouchRun("inspect", labelA)..Assert(c, icmd.Success).Stdout()

Letty5411 · 2018-06-26T09:09:40Z

test/cli_ps_test.go

+	c.Assert(exist3, check.Equals, true)
+	c.Assert(exist4, check.Equals, true)
+}
+


could also add negative tests, like:
-f null
-f wrong format

and use multiple -f

All case has add.

fuweid · 2018-06-26T13:59:09Z

apis/opts/filter.go

+	"status": true,
+
+	// TODO(huamin.thm):
+	"before":  true,


Since it is the TODO list, could we turn it into the false? or remove the key because the value is placeholder.

fuweid · 2018-06-26T14:03:13Z

apis/opts/filter.go

+	}
+
+	parsed := make(map[string][]string)
+	for _, str := range filter {


the utils contains the ConvertKVStringsToMap function. Could we use the same functionality here? DRY :) right?

it maybe not as a common function, think about in this case the parameter is map[string]bool, and can be other type, like map[int]string, and I want output include only key, maybe next time someone need output include key/value or only value.

fuweid · 2018-06-26T14:09:03Z

apis/opts/filter.go

+}
+
+// FromURLParam gets filter from url query
+func FromURLParam(param string) (map[string][]string, error) {


I don't think that we can get the meaning from the name FromURLParam. It's confusing with other functionality, for example, the parse option into map[string][]string function. So I suggest that we should use more specific name for that part.

ToURLParam also has the same problem.

Oh, good, I think about move the file api/opts/filter.go to pkg package and make it a package named filter, for one reason it not play a role as other code in package opts do, the other is thatI think the name FromURLParam and ToURLParam will make sense.

fuweid · 2018-06-26T14:18:40Z

apis/opts/filter.go

+	err := json.NewDecoder(strings.NewReader(param)).Decode(&filter)
+
+	// params from url may not passed through api, so we need validate here
+	for name := range filter {


Why not use the following function ValidateFilter ?

fuweid · 2018-06-26T14:22:02Z

apis/swagger.yml

+      - `name=container-name`
+      - `status=running`
+      - `label=key` or `label=key=value`
+      - `network=container-network`


If the pouch doesn't support the condition right now, we should remove them from the description. Just in case that we don't give the wrong information to the user :)

I do not think so, interface should think more, we just tell which not support before this, user always not care about defined field

fuweid · 2018-06-26T14:45:01Z

daemon/mgr/container_list.go

+
+	// if filterFunc is defined, we not try other filter condition
+	if fc.filterFunc != nil {
+		if match := fc.filterFunc(c); match {


change this line into if fc.filterFunc(c) { to just make it clear :)

fuweid · 2018-06-26T14:54:11Z

daemon/mgr/container_list.go

+	for k, v := range value {
+		// filter equal condition
+		if equalValue, exist := equalKV[k]; exist {
+			if equalValue == "" {


squash the two if conditions into one if equalValue == "" || equalValue == v

fuweid · 2018-06-26T15:05:04Z

daemon/mgr/container_list.go

+
+	// filter must meets all condition
+	var match, jump, statusKey = true, true, false
+	for name := range fc.condition {


I have idea about this part.

The jump boolean means we should start next round check, right? the matchKVFilter will return one boolean value if we run the filters, exist := fc.condition[field] before check. Therefore, we don't need to check the jump in the following condition.

Does it make senses to you?

I also think about this, filters, exist := fc.condition[field] if field not exist, whether filter function return true or false is not make sense. Maybe need more think.

fuweid · 2018-06-26T15:13:48Z

test/cli_ps_test.go

@@ -71,6 +72,139 @@ func (suite *PouchPsSuite) TestPsWorks(c *check.C) {

 }

+// TestPsFilterInvalid tests "pouch ps -f" invalid
+func (suite *PouchPsSuite) TestPsFilterInvalid(c *check.C) {
+	result := command.PouchRun("ps", "-f", "foo")


Next time, we can use the table-driven test to repeat the same thing.

fuweid · 2018-06-26T15:15:11Z

test/cli_ps_test.go

+	if err := json.Unmarshal([]byte(output), &result); err != nil {
+		c.Errorf("failed to decode inspect output: %v", err)
+	}
+	labelAName := result[0].ID


s/labelAName/labelAID/ maybe better

This rule is for variable, WDYT?

support pouch ps --filter, not only equal condition, but also unequal condition, like pouch ps --filter label=a!=b Fixes: #1562. Signed-off-by: Ace-Tang <aceapril@126.com>

fuweid

LGTM @YaoZengzeng PTAL about the CRI part.

YaoZengzeng · 2018-06-28T08:37:43Z

LGTM, it's all right for CRI part :)

Ace-Tang requested review from yyb196, rudyfly, YaoZengzeng, Letty5411 and fuweid June 26, 2018 03:00

yyb196 reviewed Jun 26, 2018

View reviewed changes

Letty5411 reviewed Jun 26, 2018

View reviewed changes

fuweid reviewed Jun 26, 2018

View reviewed changes

feature: support ps filter

20b0bde

support pouch ps --filter, not only equal condition, but also unequal condition, like pouch ps --filter label=a!=b Fixes: #1562. Signed-off-by: Ace-Tang <aceapril@126.com>

fuweid changed the title ~~feature: support ps filter~~ feature: support pouch ps filter Jun 28, 2018

fuweid reviewed Jun 28, 2018

View reviewed changes

fuweid merged commit 47aae21 into AliyunContainerService:master Jun 28, 2018

Ace-Tang deleted the ps_filter branch June 28, 2018 09:14

pouchrobot mentioned this pull request Jul 5, 2018

Weekly Report of PouchContainer from 2018-06-28 to 2018-07-05 #1627

Closed

ZYecho mentioned this pull request Dec 4, 2018

refactor: refactor ps filter to unify a common filter package #2529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: support pouch ps filter #1595

feature: support pouch ps filter #1595

Ace-Tang commented Jun 25, 2018

codecov-io commented Jun 26, 2018 •

edited

Loading

yyb196 Jun 26, 2018

Ace-Tang Jun 26, 2018

Letty5411 Jun 26, 2018 •

edited

Loading

Letty5411 Jun 26, 2018

Ace-Tang Jun 26, 2018

fuweid Jun 26, 2018

fuweid Jun 26, 2018

Ace-Tang Jun 27, 2018

fuweid Jun 26, 2018

Ace-Tang Jun 27, 2018 •

edited

Loading

fuweid Jun 26, 2018

fuweid Jun 26, 2018

Ace-Tang Jun 27, 2018

fuweid Jun 28, 2018

fuweid Jun 26, 2018

Ace-Tang Jun 27, 2018

fuweid Jun 26, 2018

Ace-Tang Jun 27, 2018

fuweid Jun 26, 2018

Ace-Tang Jun 27, 2018

fuweid Jun 26, 2018

Ace-Tang Jun 27, 2018

fuweid Jun 26, 2018

Ace-Tang Jun 27, 2018

fuweid left a comment

YaoZengzeng commented Jun 28, 2018

feature: support pouch ps filter #1595

feature: support pouch ps filter #1595

Conversation

Ace-Tang commented Jun 25, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Jun 26, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Letty5411 Jun 26, 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

Ace-Tang Jun 27, 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

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 left a comment

Choose a reason for hiding this comment

YaoZengzeng commented Jun 28, 2018

codecov-io commented Jun 26, 2018 •

edited

Loading

Letty5411 Jun 26, 2018 •

edited

Loading

Ace-Tang Jun 27, 2018 •

edited

Loading