Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support filter format map[string]map[string]bool and change multi label filter to And match #2812

Conversation

wangforthinker
Copy link
Contributor

Ⅰ. Describe what this PR did

Support filter format map[string]map[string]bool and change multi label filter to And match.

Ⅱ. Does this pull request fix one issue?

none.

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

add test.

Ⅳ. Describe how to verify it

by test.

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #2812 (6897026) into master (4ad5f91) will increase coverage by 0.56%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2812      +/-   ##
==========================================
+ Coverage   69.34%   69.91%   +0.56%     
==========================================
  Files         278      278              
  Lines       17397    17831     +434     
==========================================
+ Hits        12064    12466     +402     
- Misses       3983     4004      +21     
- Partials     1350     1361      +11     
Flag Coverage Δ
criv1alpha2_test 40.23% <0.00%> (+0.94%) ⬆️
integration_test_0 36.27% <0.00%> (-0.27%) ⬇️
integration_test_1 35.05% <0.00%> (-0.29%) ⬇️
integration_test_2 36.35% <90.47%> (-0.20%) ⬇️
integration_test_3 35.19% <0.00%> (-0.27%) ⬇️
node_e2e_test 34.96% <0.00%> (-0.05%) ⬇️
unittest 29.23% <80.95%> (+0.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/utils/filters/filter.go 78.84% <84.61%> (+1.34%) ⬆️
daemon/mgr/container_list.go 78.21% <100.00%> (-0.43%) ⬇️
ctrd/utils.go 78.94% <0.00%> (-4.39%) ⬇️
daemon/daemon.go 70.05% <0.00%> (+0.15%) ⬆️
ctrd/container.go 53.88% <0.00%> (+0.37%) ⬆️
daemon/mgr/container.go 60.04% <0.00%> (+0.63%) ⬆️
cri/v1alpha2/cri_utils.go 90.10% <0.00%> (+1.26%) ⬆️
cri/stream/runtime.go 70.23% <0.00%> (+2.38%) ⬆️
ctrd/client.go 71.97% <0.00%> (+2.57%) ⬆️
apis/server/utils.go 75.00% <0.00%> (+3.84%) ⬆️
... and 4 more

@wangforthinker wangforthinker force-pushed the feature/support-filter-new-format branch 2 times, most recently from 0b0dc31 to c324529 Compare April 28, 2019 06:01
…el filter to AND match

Signed-off-by: allen.wang <allen.wq@alipay.com>
@ZYecho
Copy link
Contributor

ZYecho commented Apr 29, 2019

could you explain why we need to change multi label filter to And match? the behavior in docker I think is OR match when use multi filter, should we be consistent with docker? @wangforthinker

@allencloud
Copy link
Collaborator

Any update on this? @wangforthinker
Could you give some feedback on @ZYecho 's question?

@wangforthinker
Copy link
Contributor Author

could you explain why we need to change multi label filter to And match? the behavior in docker I think is OR match when use multi filter, should we be consistent with docker? @wangforthinker

Thanks for your question. As I know, label multi filter in docker is AND match. And the behavior is more suitable for daily usage habits. @ZYecho

@wangforthinker wangforthinker reopened this May 5, 2019
@ZYecho
Copy link
Contributor

ZYecho commented May 5, 2019

could you explain why we need to change multi label filter to And match? the behavior in docker I think is OR match when use multi filter, should we be consistent with docker? @wangforthinker

Thanks for your question. As I know, label multi filter in docker is AND match. And the behavior is more suitable for daily usage habits. @ZYecho

Yep I think you're right. the multi label filter was AND match, but multi reference/name filter was OR match.
It's a little wired, I open an issue in moby.

@wangforthinker
Copy link
Contributor Author

@HusterWan @allencloud Could you review this PR?

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cardyok cardyok requested a review from cxz66666 July 7, 2022 12:15
@cxz66666
Copy link
Collaborator

cxz66666 commented Jul 9, 2022

Thanks for your contribution! At now there is no progress about this issue, so I think we can be consistent with docker. But can you explain about why we need support map[string]map[string]bool format? After getting your response, I will review and merge this pr ASAP!

Copy link
Collaborator

@cxz66666 cxz66666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@cxz66666 cxz66666 merged commit 6af4c26 into AliyunContainerService:master Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants