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/09/10 08:02:37 UTC
[GitHub] [apisix] Firstsawyou opened a new pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Firstsawyou opened a new pull request #2209:
URL: https://github.com/apache/apisix/pull/2209
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
feature: #2202
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible?
----------------------------------------------------------------
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] moonming commented on pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#issuecomment-690066620
Is this PR still a draft?
----------------------------------------------------------------
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] membphis commented on pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#issuecomment-690114593
@Firstsawyou this PR is missing test case. if it is still in process, you should choose `draft` when you create new PR.
pls take a look at the output of CI:
https://github.com/apache/apisix/pull/2209/checks?check_run_id=1095273514#step:6:299
----------------------------------------------------------------
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] Firstsawyou commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r489496814
##########
File path: t/admin/routes.t
##########
@@ -1557,8 +1557,7 @@ location /t {
ngx.say("code: ", code)
ngx.say(body)
- -- etcd v3 would still get the value at 2s, don't know why yet
- ngx.sleep(2.5)
+ ngx.sleep(2)
Review comment:
remember I haven't changed this.
----------------------------------------------------------------
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] moonming commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r487604738
##########
File path: t/admin/schema.t
##########
@@ -40,7 +40,7 @@ qr/"plugins":\{"type":"object"}/
--- request
GET /apisix/admin/schema/service
--- response_body eval
-qr/"required":\["upstream"\]/
+qr/"required".*additionalProperties/
Review comment:
why change this line?
##########
File path: t/admin/services.t
##########
@@ -1085,3 +1085,50 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 31: set service(id: 1)（allow empty `service` object）
Review comment:
Insufficient test cases, can I add content in the empty object later?
----------------------------------------------------------------
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] Firstsawyou commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r488819427
##########
File path: t/admin/schema.t
##########
@@ -37,10 +37,47 @@ qr/"plugins":\{"type":"object"}/
=== TEST 2: get service schema
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/schema/service',
+ ngx.HTTP_GET,
+ "",
+ [[{
+ "anyOf": [
+ {
+ "required":[
+ "upstream"
+ ]
+ },
+ {
+ "required":[
+ "upstream_id"
+ ]
+ },
+ {
+ "required":[
+ "plugins"
+ ]
+ },
+ {
+ "required":[
+ "script"
+ ]
+ }
+ ],
+ "action": "set"
+ }]]
Review comment:
The way to detect the removal of "anyOf" is wrong, causing the test to fail.
----------------------------------------------------------------
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] Firstsawyou commented on pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#issuecomment-690118556
> Is this PR still a draft?
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
[GitHub] [apisix] moonming commented on pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#issuecomment-690119215
please add the WIP or draft title.
Thanks,
Ming Wen
Twitter: _WenMing
Firstsawyou <no...@github.com> 于2020年9月10日周四 下午5:44写道：
> Is this PR still a draft?
>
> yes.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2209#issuecomment-690118556>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBK5RIYYUUNEMLBK3FDDSFCNXNANCNFSM4REP5E2A>
> .
>
----------------------------------------------------------------
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] membphis commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r489481394
##########
File path: t/admin/routes.t
##########
@@ -1611,7 +1610,7 @@ location /t {
end
ngx.say("[push] succ: ", body)
- ngx.sleep(2.5)
+ ngx.sleep(2)
Review comment:
ditto
##########
File path: t/admin/routes.t
##########
@@ -1557,8 +1557,7 @@ location /t {
ngx.say("code: ", code)
ngx.say(body)
- -- etcd v3 would still get the value at 2s, don't know why yet
- ngx.sleep(2.5)
+ ngx.sleep(2)
Review comment:
why change this?
----------------------------------------------------------------
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] membphis commented on a change in pull request #2209: draft: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r487732038
##########
File path: t/admin/services.t
##########
@@ -1085,3 +1085,50 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 31: set service(id: 1)（allow empty `service` object）
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "desc": "empty service"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 32: get service(id: 1)（allow empty `service` object)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_GET
+ )
Review comment:
we need to check the response body, eg `desc` field
----------------------------------------------------------------
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] membphis commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r489450650
##########
File path: t/admin/routes.t
##########
@@ -2180,3 +2180,65 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 60: set empty service. (id: 1)（allow empty `service` object）
Review comment:
we can not do `E2E` test in folder `t/admin/*`.
please move those test case to `t/node/`, `service-empty.t` is a good test file name.
----------------------------------------------------------------
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] Firstsawyou commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r489452597
##########
File path: t/admin/routes.t
##########
@@ -2180,3 +2180,65 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 60: set empty service. (id: 1)（allow empty `service` object）
Review comment:
ok.
----------------------------------------------------------------
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] membphis commented on pull request #2209: draft: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#issuecomment-691020500
----------------------------------------------------------------
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] Firstsawyou commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r487616802
##########
File path: t/admin/services.t
##########
@@ -1085,3 +1085,50 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 31: set service(id: 1)（allow empty `service` object）
Review comment:
OK, I will add the complete test case.
----------------------------------------------------------------
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] membphis commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r489451763
##########
File path: t/admin/routes.t
##########
@@ -2180,3 +2180,65 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 60: set empty service. (id: 1)（allow empty `service` object）
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ {},
+ [[{
+ "node": {
+ "value": {"id":"1"}
+ },
+ "action": "set"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 61: route binding empty service
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "methods": ["GET"],
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:8080": 1
Review comment:
why `8080`?
----------------------------------------------------------------
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] Firstsawyou commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r488599115
##########
File path: t/admin/services.t
##########
@@ -1085,3 +1085,50 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 31: set service(id: 1)（allow empty `service` object）
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ [[{
+ "desc": "empty service"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 32: get service(id: 1)（allow empty `service` object)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_GET
+ )
Review comment:
ok.
----------------------------------------------------------------
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] Firstsawyou commented on pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#issuecomment-690118919
> @Firstsawyou this PR is missing test case. if it is still in process, you should choose `draft` when you create new PR.
>
> pls take a look at the output of CI:
>
> https://github.com/apache/apisix/pull/2209/checks?check_run_id=1095273514#step:6:299
okay.
----------------------------------------------------------------
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] membphis commented on pull request #2209: draft: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#issuecomment-691020500
test case: success to create an empty service
----------------------------------------------------------------
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] membphis commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r489452341
##########
File path: t/admin/routes.t
##########
@@ -2180,3 +2180,65 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 60: set empty service. (id: 1)（allow empty `service` object）
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ {},
+ [[{
+ "node": {
+ "value": {"id":"1"}
+ },
+ "action": "set"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 61: route binding empty service
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "methods": ["GET"],
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:8080": 1
Review comment:
take a look at: https://github.com/apache/apisix/blob/master/t/node/route-domain.t#L96
----------------------------------------------------------------
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] Firstsawyou commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r487616083
##########
File path: t/admin/schema.t
##########
@@ -40,7 +40,7 @@ qr/"plugins":\{"type":"object"}/
--- request
GET /apisix/admin/schema/service
--- response_body eval
-qr/"required":\["upstream"\]/
+qr/"required".*additionalProperties/
Review comment:
Because after removing the "required" of service in the schema_def.lua file, it will not match `qr/"required":\["upstream"\]/ `.
----------------------------------------------------------------
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] Firstsawyou closed pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
Firstsawyou closed pull request #2209:
URL: https://github.com/apache/apisix/pull/2209
----------------------------------------------------------------
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] membphis commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r489260881
##########
File path: t/admin/services.t
##########
@@ -1085,3 +1085,97 @@ GET /t
--- error_code: 400
--- no_error_log
[error]
+
+
+
+=== TEST 31: set empty service. (id: 1)（allow empty `service` object）
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/1',
+ ngx.HTTP_PUT,
+ {},
+ [[{
+ "node": {
+ "value": {"id":"1"}
+ },
+ "action": "set"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 32: add content to the empty service.(id: 1)（allow empty `service` object）
Review comment:
bad title
##########
File path: t/admin/schema.t
##########
@@ -37,10 +37,29 @@ qr/"plugins":\{"type":"object"}/
=== TEST 2: get service schema
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local t = require("lib.test_admin").test
+ local code, _, res_body = t('/apisix/admin/schema/service',
+ ngx.HTTP_GET
+ )
+ local res_data = core.json.decode(res_body)
+ local result = res_data["anyOf"]
Review comment:
bad process, not easy to read
----------------------------------------------------------------
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] membphis commented on a change in pull request #2209: draft: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r487731432
##########
File path: t/admin/schema.t
##########
@@ -40,7 +40,7 @@ qr/"plugins":\{"type":"object"}/
--- request
GET /apisix/admin/schema/service
--- response_body eval
-qr/"required":\["upstream"\]/
+qr/"required".*additionalProperties/
Review comment:
this way is not easy to confirm if we removed `anyOf`, we should check it in Lua land
----------------------------------------------------------------
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] membphis commented on pull request #2209: draft: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#issuecomment-691020500
----------------------------------------------------------------
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] membphis commented on a change in pull request #2209: feature: allow empty `service` object, it does not contain any `upstream` and `plugin`.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2209:
URL: https://github.com/apache/apisix/pull/2209#discussion_r489500949
##########
File path: t/admin/routes.t
##########
@@ -1557,8 +1557,7 @@ location /t {
ngx.say("code: ", code)
ngx.say(body)
- -- etcd v3 would still get the value at 2s, don't know why yet
- ngx.sleep(2.5)
+ ngx.sleep(2)
Review comment:
please revert them
----------------------------------------------------------------
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