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 2022/06/06 15:55:42 UTC
[GitHub] [apisix] zhendongcmss opened a new pull request, #7201: fix: add debug yaml validation
zhendongcmss opened a new pull request, #7201:
URL: https://github.com/apache/apisix/pull/7201
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue) https://github.com/apache/apisix/issues/7159
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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 diff in pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7201:
URL: https://github.com/apache/apisix/pull/7201#discussion_r894129365
##########
t/debug/hook.t:
##########
@@ -120,10 +125,35 @@ hook_test: # module and function list, name: hook_test
GET /hello
--- more_headers
Host: foo.com
+X-APISIX-Dynamic-Debug: true
--- response_body
hello world
---- no_error_log
-[error]
--- error_log
filter(): call require("apisix.plugin").filter() args:{
filter(): call require("apisix.plugin").filter() return:{
+
+
+
+=== TEST 5: missing hook_conf
+--- debug_config
+basic:
+ enable: true
+http_filter:
+ enable: true # enable or disable this feature
+ enable_header_name: X-APISIX-Dynamic-Debug # the header name of dynamic enable
+
+hook_test: # module and function list, name: hook_test
+ apisix.plugin: # required module name
+ - filter # function name
+
+#END
+--- request
+GET /hello
+--- more_headers
+Host: foo.com
+X-APISIX-Dynamic-Debug: true
+--- response_body
+hello world
+--- error_log
+read_debug_yaml(): failed to validate debug config property "hook_conf" is required
+--- wait: 3
Review Comment:
I think `wait: 2` is enough as we load debug yaml per second: https://github.com/apache/apisix/blob/f2326af32233442289cf9783f1022bfc5e532ca3/apisix/debug.lua#L311
--
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] zhendongcmss commented on pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #7201:
URL: https://github.com/apache/apisix/pull/7201#issuecomment-1150593556
> > failed to validate debug config property "hook_conf" is required
>
> The error message just explained the failure reason.
Actually, them match, why test case doesn't 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] spacewander commented on a diff in pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7201:
URL: https://github.com/apache/apisix/pull/7201#discussion_r893012680
##########
apisix/debug.lua:
##########
@@ -204,8 +260,9 @@ local function sync_debug_status(premature)
return
end
- read_debug_yaml()
- sync_debug_hooks()
+ if read_debug_yaml() then
Review Comment:
```
if not ... then
return
end
```
would be better?
##########
t/debug/hook.t:
##########
@@ -127,3 +132,28 @@ hello world
--- error_log
filter(): call require("apisix.plugin").filter() args:{
filter(): call require("apisix.plugin").filter() return:{
+
+
+=== TEST 5: missing hook_conf
+--- debug_config
+basic:
+ enable: true
+http_filter:
+ enable: true # enable or disable this feature
+ enable_header_name: X-APISIX-Dynamic-Debug # the header name of dynamic enable
+
+hook_test: # module and function list, name: hook_test
+ apisix.plugin: # required module name
+ - filter # function name
+
+#END
+--- request
+GET /hello
+--- more_headers
+Host: foo.com
+--- response_body
+hello world
+--- no_error_log
+[error]
Review Comment:
We can't check there is no `[error]` because `failed to validate debug config property "hook_conf" is required` is an error level log.
--
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 #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7201:
URL: https://github.com/apache/apisix/pull/7201#issuecomment-1148114165
Please make the CI pass, thanks!
--
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 diff in pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7201:
URL: https://github.com/apache/apisix/pull/7201#discussion_r894148664
##########
t/debug/hook.t:
##########
@@ -120,10 +125,35 @@ hook_test: # module and function list, name: hook_test
GET /hello
--- more_headers
Host: foo.com
+X-APISIX-Dynamic-Debug: true
--- response_body
hello world
---- no_error_log
-[error]
--- error_log
filter(): call require("apisix.plugin").filter() args:{
filter(): call require("apisix.plugin").filter() return:{
+
+
+
+=== TEST 5: missing hook_conf
+--- debug_config
+basic:
+ enable: true
+http_filter:
+ enable: true # enable or disable this feature
+ enable_header_name: X-APISIX-Dynamic-Debug # the header name of dynamic enable
+
+hook_test: # module and function list, name: hook_test
+ apisix.plugin: # required module name
+ - filter # function name
+
+#END
+--- request
+GET /hello
+--- more_headers
+Host: foo.com
+X-APISIX-Dynamic-Debug: true
+--- response_body
+hello world
+--- error_log
+read_debug_yaml(): failed to validate debug config property "hook_conf" is required
+--- wait: 3
Review Comment:
Anyway, wait for one more seconds is acceptable
--
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] zhendongcmss commented on pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #7201:
URL: https://github.com/apache/apisix/pull/7201#issuecomment-1148424094
@spacewander The CI test is not stable, is there a way to restart the CI test without git push?
--
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 #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7201:
URL: https://github.com/apache/apisix/pull/7201#issuecomment-1149497263
https://github.com/apache/apisix/runs/6785395449?check_suite_focus=true
> init_worker_by_lua error: /home/runner/work/apisix/apisix/apisix/debug.lua:225: attempt to index local 'hook_conf' (a nil value)"
It seems this causes the CI failure.
--
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 #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7201:
URL: https://github.com/apache/apisix/pull/7201
--
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] zhendongcmss commented on pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #7201:
URL: https://github.com/apache/apisix/pull/7201#issuecomment-1149764608
I got the eror, is there way to let it pass ?
https://github.com/apache/apisix/runs/6790365518?check_suite_focus=true#step:15:704
```
Error: led test 't/debug/hook.t TEST 5: missing hook_conf - pattern "[error]" should not match any line in error.log but matches line "2022/06/08 09:45:54 [error] 18297#18297: *2 [lua] debug.lua:147: read_debug_yaml(): failed to validate debug config property \"hook_conf\" is required, context: init_worker_by_lua*" (req 0)
```
--
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 pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7201:
URL: https://github.com/apache/apisix/pull/7201#issuecomment-1150565753
> failed to validate debug config property \"hook_conf\" is required
The error message just explained the failure reason.
--
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 pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7201:
URL: https://github.com/apache/apisix/pull/7201#issuecomment-1149334043
> @spacewander The CI test is not stable, is there a way to restart the CI test without git push?
I just re-ran 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on pull request #7201: fix: add debug yaml validation
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #7201:
URL: https://github.com/apache/apisix/pull/7201#issuecomment-1149766322
> > @spacewander The CI test is not stable, is there a way to restart the CI test without git push?
>
> I just re-ran them.
How to re-run ? I don't found the re-run button.
--
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