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 2021/12/07 10:29:37 UTC
[GitHub] [apisix] zuiyangqingzhou opened a new pull request #5728: refine the ua-restriction plugin configuration checksum logic
zuiyangqingzhou opened a new pull request #5728:
URL: https://github.com/apache/apisix/pull/5728
fix: [5724](https://github.com/apache/apisix/issues/5724)
### 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. -->
### Pre-submission checklist:
<!--
Please follow the requirements:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. Use "request review" to notify the reviewer once you have resolved the review
-->
* [ ] 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? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #5728: fix(ua-restriction): refine plugin configuration check logic
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #5728:
URL: https://github.com/apache/apisix/pull/5728
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zuiyangqingzhou commented on a change in pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
zuiyangqingzhou commented on a change in pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#discussion_r768283717
##########
File path: t/plugin/ua-restriction.t
##########
@@ -739,3 +739,51 @@ hello world
}
--- response_body
passed
+
+
+
+=== TEST 33: the element in allowList is null
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.ua-restriction")
+ local conf = {
+ allowlist = {
+ null
+ },
+ }
+ local ok, err = plugin.check_schema(conf)
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+property "allowlist" validation failed: expect array to have at least 1 items
Review comment:
No problem
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#issuecomment-996338635
@zuiyangqingzhou
Would you update the test to make CI pass?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zuiyangqingzhou commented on pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
zuiyangqingzhou commented on pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#issuecomment-997155763
This should be caused by something else
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zuiyangqingzhou commented on a change in pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
zuiyangqingzhou commented on a change in pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#discussion_r771922260
##########
File path: t/plugin/ua-restriction.t
##########
@@ -783,5 +790,7 @@ load_full_data(): failed to check item data of [/apisix/routes] err:failed to ch
ngx.say("done")
}
}
---- error_log
-load_full_data(): failed to check item data of [/apisix/routes] err:failed to check the configuration of plugin ua-restriction
+--- response_body
+property "denylist" validation failed: wrong type: expected array, got table
+done
+--- ONLY
Review comment:
yes, it has been deleted
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#issuecomment-997370246
I rerun the CI right now. one of the ci failed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zuiyangqingzhou commented on pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
zuiyangqingzhou commented on pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#issuecomment-990795264
> Could you add a test in https://github.com/apache/apisix/blob/master/t/plugin/ua-restriction.t?
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on a change in pull request #5728: refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#discussion_r763979941
##########
File path: apisix/plugins/ua-restriction.lua
##########
@@ -34,11 +35,11 @@ local schema = {
type = "boolean",
default = false,
},
- allowlist = {
+ allow_list = {
type = "array",
minItems = 1
},
- denylist = {
+ deny_list = {
Review comment:
May I know the reason to modify it?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on a change in pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#discussion_r765336750
##########
File path: apisix/plugins/ua-restriction.lua
##########
@@ -34,11 +35,11 @@ local schema = {
type = "boolean",
default = false,
},
- allowlist = {
+ allow_list = {
type = "array",
minItems = 1
},
- denylist = {
+ deny_list = {
Review comment:
sure, we have a code style file 😄 https://github.com/apache/apisix/blob/master/CODE_STYLE.md#variable
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#discussion_r767264897
##########
File path: t/plugin/ua-restriction.t
##########
@@ -739,3 +739,51 @@ hello world
}
--- response_body
passed
+
+
+
+=== TEST 33: the element in allowList is null
+--- config
+ location /t {
+ content_by_lua_block {
+ local plugin = require("apisix.plugins.ua-restriction")
+ local conf = {
+ allowlist = {
+ null
+ },
+ }
+ local ok, err = plugin.check_schema(conf)
+ if not ok then
+ ngx.say(err)
+ end
+
+ ngx.say("done")
+ }
+ }
+--- response_body
+property "allowlist" validation failed: expect array to have at least 1 items
Review comment:
Look like the conf is not rejected because of the bad regex? Maybe we need a wrong regex to test it.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zuiyangqingzhou commented on pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
zuiyangqingzhou commented on pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#issuecomment-997155462
> @zuiyangqingzhou Would you update the test to make CI pass?
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on pull request #5728: refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#issuecomment-987915927
@zuiyangqingzhou Hi, please also update this PR's description 😄
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #5728: fix(ua-restriction): refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#discussion_r771885455
##########
File path: t/plugin/ua-restriction.t
##########
@@ -783,5 +790,7 @@ load_full_data(): failed to check item data of [/apisix/routes] err:failed to ch
ngx.say("done")
}
}
---- error_log
-load_full_data(): failed to check item data of [/apisix/routes] err:failed to check the configuration of plugin ua-restriction
+--- response_body
+property "denylist" validation failed: wrong type: expected array, got table
+done
+--- ONLY
Review comment:
need to remove this line
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #5728: refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#discussion_r763952504
##########
File path: apisix/plugins/ua-restriction.lua
##########
@@ -34,11 +35,11 @@ local schema = {
type = "boolean",
default = false,
},
- allowlist = {
+ allow_list = {
Review comment:
It's a broken change.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zuiyangqingzhou commented on a change in pull request #5728: refine the ua-restriction plugin configuration checksum logic
Posted by GitBox <gi...@apache.org>.
zuiyangqingzhou commented on a change in pull request #5728:
URL: https://github.com/apache/apisix/pull/5728#discussion_r764585950
##########
File path: apisix/plugins/ua-restriction.lua
##########
@@ -34,11 +35,11 @@ local schema = {
type = "boolean",
default = false,
},
- allowlist = {
+ allow_list = {
type = "array",
minItems = 1
},
- denylist = {
+ deny_list = {
Review comment:
Oh, I just don't think it's in line with the variable naming specification, and if the scope of the impact is too great, I'll change it back @juzhiyuan
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org