You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/12/30 03:45:46 UTC
[GitHub] [apisix-dashboard] starsz opened a new pull request #1151: fix: search labels with the same key
starsz opened a new pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151
Please answer these questions before submitting a pull request
- Why submit this pull request?
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- Related issues
fix #1130
___
### Bugfix
- Description
If the label argument with the same key, the previous value will be covered by the latter value in genLabelMap.
So the response is not correct.
- How to fix?
Use the `key:value` as a key in genLabelMap.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] juzhiyuan commented on a change in pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151#discussion_r550177929
##########
File path: api/internal/handler/label/label.go
##########
@@ -78,15 +78,18 @@ type ListInput struct {
Label string `auto_read:"label,query"`
}
-func subsetOf(reqLabels, labels map[string]string) map[string]string {
+func subsetOf(reqLabels map[string]struct{}, labels map[string]string) map[string]string {
if len(reqLabels) == 0 {
return labels
}
var res = make(map[string]string)
for k, v := range labels {
- l, exist := reqLabels[k]
- if exist && ((l == "") || v == l) {
+ if _, exist := reqLabels[k]; exist {
+ res[k] = v
+ }
+
+ if _, exist := reqLabels[k+":"+v]; exist {
Review comment:
`[k+":"+v]` formmated?
##########
File path: api/test/e2e/label_test.go
##########
@@ -200,6 +200,16 @@ func TestLabel(t *testing.T) {
ExpectStatus: http.StatusOK,
ExpectBody: "{\"build\":\"16\"},{\"build\":\"17\"}",
},
+ {
+ Desc: "get labels with the same key (key = build)",
+ Object: ManagerApiExpect(t),
+ Method: http.MethodGet,
+ Headers: map[string]string{"Authorization": token},
+ Query: "label=build:16,build:17",
+ Path: "/apisix/admin/labels/all",
Review comment:
🤔 What will happen if calling `/labels` only?
##########
File path: api/test/e2e/route_with_management_fileds_test.go
##########
@@ -333,6 +333,17 @@ func TestRoute_search_by_label(t *testing.T) {
ExpectBody: "\"total_size\":2",
Sleep: sleepTime,
},
+ {
+ Desc: "search the route by label (combination)",
+ Object: ManagerApiExpect(t),
+ Path: "/apisix/admin/routes",
+ Query: "label=build:16,build:17",
+ Method: http.MethodGet,
+ Headers: map[string]string{"Authorization": token},
+ ExpectStatus: http.StatusOK,
+ ExpectBody: "\"total_size\":2",
Review comment:
just to confirm, is this all contents in response?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151#discussion_r550222467
##########
File path: api/internal/handler/label/label.go
##########
@@ -78,15 +78,18 @@ type ListInput struct {
Label string `auto_read:"label,query"`
}
-func subsetOf(reqLabels, labels map[string]string) map[string]string {
+func subsetOf(reqLabels map[string]struct{}, labels map[string]string) map[string]string {
if len(reqLabels) == 0 {
return labels
}
var res = make(map[string]string)
for k, v := range labels {
- l, exist := reqLabels[k]
- if exist && ((l == "") || v == l) {
+ if _, exist := reqLabels[k]; exist {
+ res[k] = v
+ }
+
+ if _, exist := reqLabels[k+":"+v]; exist {
Review comment:
Yes.It's the behavior of IDE.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151#discussion_r550375749
##########
File path: api/test/e2e/label_test.go
##########
@@ -200,6 +200,16 @@ func TestLabel(t *testing.T) {
ExpectStatus: http.StatusOK,
ExpectBody: "{\"build\":\"16\"},{\"build\":\"17\"}",
},
+ {
+ Desc: "get labels with the same key (key = build)",
+ Object: ManagerApiExpect(t),
+ Method: http.MethodGet,
+ Headers: map[string]string{"Authorization": token},
+ Query: "label=build:16,build:17",
+ Path: "/apisix/admin/labels/all",
Review comment:
It will return `{"error_msg":"404 Route Not Found"}`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] codecov-io edited a comment on pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151#issuecomment-752317766
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=h1) Report
> Merging [#1151](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=desc) (6cbd8fa) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/2dea0251d2f3cd215aa0a9fa29cc7656ec0d120e?el=desc) (2dea025) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1151 +/- ##
==========================================
+ Coverage 42.32% 42.38% +0.05%
==========================================
Files 31 31
Lines 1928 1930 +2
==========================================
+ Hits 816 818 +2
Misses 1001 1001
Partials 111 111
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/handler/label/label.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1151/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvbGFiZWwvbGFiZWwuZ28=) | `70.29% <100.00%> (+0.29%)` | :arrow_up: |
| [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1151/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `57.81% <100.00%> (+0.66%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=footer). Last update [2dea025...6cbd8fa](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] codecov-io commented on pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151#issuecomment-752317766
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=h1) Report
> Merging [#1151](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=desc) (2e73815) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/4317a5f44cc0bc6968b505bdc90045e2d081ba42?el=desc) (4317a5f) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1151 +/- ##
=======================================
Coverage 42.30% 42.31%
=======================================
Files 31 31
Lines 1924 1926 +2
=======================================
+ Hits 814 815 +1
- Misses 1000 1001 +1
Partials 110 110
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [api/internal/handler/label/label.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1151/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvbGFiZWwvbGFiZWwuZ28=) | `70.29% <100.00%> (+0.29%)` | :arrow_up: |
| [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1151/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `57.81% <100.00%> (+0.66%)` | :arrow_up: |
| [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1151/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `79.37% <0.00%> (-0.63%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=footer). Last update [4317a5f...2e73815](https://codecov.io/gh/apache/apisix-dashboard/pull/1151?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] nic-chen merged pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
nic-chen merged pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151#discussion_r550375749
##########
File path: api/test/e2e/label_test.go
##########
@@ -200,6 +200,16 @@ func TestLabel(t *testing.T) {
ExpectStatus: http.StatusOK,
ExpectBody: "{\"build\":\"16\"},{\"build\":\"17\"}",
},
+ {
+ Desc: "get labels with the same key (key = build)",
+ Object: ManagerApiExpect(t),
+ Method: http.MethodGet,
+ Headers: map[string]string{"Authorization": token},
+ Query: "label=build:16,build:17",
+ Path: "/apisix/admin/labels/all",
Review comment:
It will return 404.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
starsz commented on pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151#issuecomment-752386572
> We use `OR` or `AND` when there are multiple conditions?
OR. It is used for filter routes by label.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1151: fix: search labels with the same key
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1151:
URL: https://github.com/apache/apisix-dashboard/pull/1151#discussion_r550376235
##########
File path: api/test/e2e/route_with_management_fileds_test.go
##########
@@ -333,6 +333,17 @@ func TestRoute_search_by_label(t *testing.T) {
ExpectBody: "\"total_size\":2",
Sleep: sleepTime,
},
+ {
+ Desc: "search the route by label (combination)",
+ Object: ManagerApiExpect(t),
+ Path: "/apisix/admin/routes",
+ Query: "label=build:16,build:17",
+ Method: http.MethodGet,
+ Headers: map[string]string{"Authorization": token},
+ ExpectStatus: http.StatusOK,
+ ExpectBody: "\"total_size\":2",
Review comment:
Yes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org