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/09/28 06:16:35 UTC
[GitHub] [apisix] kingluo opened a new pull request, #8006: feat(test): add default no_error_log
kingluo opened a new pull request, #8006:
URL: https://github.com/apache/apisix/pull/8006
### Description
add default `no_error_log` if no `error_log` or `no_error_log` defined
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] 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] tzssangglass commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1005153130
##########
t/control/healthcheck.t:
##########
@@ -48,6 +35,13 @@ run_tests;
__DATA__
=== TEST 1: upstreams
+--- yaml_config
+apisix:
+ node_listen: 1984
+deployment:
+ role: data_plane
+ role_data_plane:
+ config_provider: yaml
--- apisix_yaml
Review Comment:
we can't keep this in the `add_block_preprocessor`?
--
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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1027297147
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -224,6 +230,8 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+not found environment variable
Review Comment:
ditto.
--
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] tzssangglass commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1005153358
##########
t/control/healthcheck.t:
##########
@@ -48,6 +35,13 @@ run_tests;
__DATA__
=== TEST 1: upstreams
+--- yaml_config
+apisix:
+ node_listen: 1984
+deployment:
+ role: data_plane
+ role_data_plane:
+ config_provider: yaml
Review Comment:
we can't keep this in the `add_block_preprocessor`?
--
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 #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1005264617
##########
t/control/discovery.t:
##########
@@ -77,6 +73,8 @@ GET /t
--- response_body
{}
{"fetch_interval":3,"keepalive":true,"prefix":"upstreams","servers":["http://127.0.0.1:8500","http://127.0.0.1:8600"],"timeout":{"connect":2000,"read":2000,"wait":60},"weight":1}
+--- error_log
+connect consul
Review Comment:
@kingluo
we should add some comment for this case.
##########
t/control/discovery.t:
##########
@@ -77,6 +73,8 @@ GET /t
--- response_body
{}
{"fetch_interval":3,"keepalive":true,"prefix":"upstreams","servers":["http://127.0.0.1:8500","http://127.0.0.1:8600"],"timeout":{"connect":2000,"read":2000,"wait":60},"weight":1}
+--- error_log
+connect consul
Review Comment:
@kingluo
we should add some comments for this 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1001481854
##########
t/plugin/dubbo-proxy/upstream.t:
##########
@@ -88,6 +88,7 @@ routes:
#END
--- response_body
dubbo success
+--- ignore_error_log
Review Comment:
Becuase this case is just to test retry, which would print error log for retry failed. The error logs are expected, so should be ignored.
--
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 #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1026507185
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -286,3 +294,5 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+failed to get lua_shared_dict
Review Comment:
We can use `--- http_config` to provide such configuration if we can't generate them automatically.
##########
t/control/discovery.t:
##########
@@ -77,6 +73,8 @@ GET /t
--- response_body
{}
{"fetch_interval":3,"keepalive":true,"prefix":"upstreams","servers":["http://127.0.0.1:8500","http://127.0.0.1:8600"],"timeout":{"connect":2000,"read":2000,"wait":60},"weight":1}
+--- error_log
+connect consul
Review Comment:
What about adding the comment at the top of this file?
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -224,6 +230,8 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+not found environment variable
Review Comment:
We can use `--- main_config` to provide dummy env var
--
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 #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1001393797
##########
t/discovery/consul_kv.t:
##########
@@ -218,6 +218,7 @@ routes:
GET /hello
--- response_body_like eval
qr/server [1-2]/
+--- ignore_error_log
Review Comment:
Why do we need to ignore error log in this file?
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -224,6 +230,8 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+not found environment variable
Review Comment:
Could we provide dummy env var in this file to suppress repeated error msg?
##########
t/plugin/kafka-logger.t:
##########
@@ -148,6 +144,7 @@ GET /hello
--- response_body
hello world
--- wait: 2
+--- ignore_error_log
Review Comment:
Ditto
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -286,3 +294,5 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+failed to get lua_shared_dict
Review Comment:
Would be better to add required shdict in the test nginx.conf?
##########
t/plugin/dubbo-proxy/upstream.t:
##########
@@ -88,6 +88,7 @@ routes:
#END
--- response_body
dubbo success
+--- ignore_error_log
Review Comment:
Why do we add ignore_error_log here?
##########
t/control/discovery.t:
##########
@@ -77,6 +73,8 @@ GET /t
--- response_body
{}
{"fetch_interval":3,"keepalive":true,"prefix":"upstreams","servers":["http://127.0.0.1:8500","http://127.0.0.1:8600"],"timeout":{"connect":2000,"read":2000,"wait":60},"weight":1}
+--- error_log
+connect consul
Review Comment:
Why do we need to check "connect consul" error 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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1001479968
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -224,6 +230,8 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+not found environment variable
Review Comment:
ditto.
--
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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1001474506
##########
t/control/discovery.t:
##########
@@ -77,6 +73,8 @@ GET /t
--- response_body
{}
{"fetch_interval":3,"keepalive":true,"prefix":"upstreams","servers":["http://127.0.0.1:8500","http://127.0.0.1:8600"],"timeout":{"connect":2000,"read":2000,"wait":60},"weight":1}
+--- error_log
+connect consul
Review Comment:
Because this whole test file is only used to verify the configuration set or not, but the configuratioin content is invalid, which contains non-exist consul server address.
--
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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1019959888
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -286,3 +294,5 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+failed to get lua_shared_dict
Review Comment:
Cannot. Because the shared_dict directives for k8s discovery is generated in normal APISIX startup, but not APISIX.pm.
This test case is only for configuration comparison, not to use the configuration.
--
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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1019960261
##########
t/control/discovery.t:
##########
@@ -77,6 +73,8 @@ GET /t
--- response_body
{}
{"fetch_interval":3,"keepalive":true,"prefix":"upstreams","servers":["http://127.0.0.1:8500","http://127.0.0.1:8600"],"timeout":{"connect":2000,"read":2000,"wait":60},"weight":1}
+--- error_log
+connect consul
Review Comment:
`--- error_log` does not support comment.
--
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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1001479055
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -286,3 +294,5 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+failed to get lua_shared_dict
Review Comment:
This test case is only to compare two configuration, not to test real k8s discovery functioni. I think it's ok to ignore the side-effect triggered by invalid configuration content.
--
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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1005251194
##########
t/control/healthcheck.t:
##########
@@ -48,6 +35,13 @@ run_tests;
__DATA__
=== TEST 1: upstreams
+--- yaml_config
+apisix:
+ node_listen: 1984
+deployment:
+ role: data_plane
+ role_data_plane:
+ config_provider: yaml
Review Comment:
This test file does not use yaml_config at all.
--
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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1027297114
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -286,3 +294,5 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+failed to get lua_shared_dict
Review Comment:
This test file is only used to compare if the configuration is set as expected, it does not care about whether the k8s function works or not, and in fact, the configuration is invalid and needs to get other auxiliary settings, but that is done by kubernetes2.t and kubernetes3.t. You have to repeat everything from those test files into this test file to shut up the errors, but it's meaningless because it's not the goal of this test file.
So all in all, no need to be sensitive to the error log here, as long as the response is ok.
So now I choose to add `--- ignore_error_log` here.
--
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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1019999749
##########
t/discovery/consul_kv.t:
##########
@@ -218,6 +218,7 @@ routes:
GET /hello
--- response_body_like eval
qr/server [1-2]/
+--- ignore_error_log
Review Comment:
I check all `--- ignore_error_log`, and I confirm they are necessary because retries of these test cases occurrences are random, so you could not assert they happen or not. After all, they are just retries, and do not affect the test goals, so we have to ignore 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] kingluo commented on a diff in pull request #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1020034485
##########
t/discovery/consul_kv.t:
##########
@@ -218,6 +218,7 @@ routes:
GET /hello
--- response_body_like eval
qr/server [1-2]/
+--- ignore_error_log
Review Comment:
And some are health checker retries.
--
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 #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #8006:
URL: https://github.com/apache/apisix/pull/8006
--
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 #8006: feat(test): add default no_error_log
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8006:
URL: https://github.com/apache/apisix/pull/8006#discussion_r1005265221
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -286,3 +294,5 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+failed to get lua_shared_dict
Review Comment:
But providing a correct conf would be better, instead of using a wrong conf and ignore the error.
##########
t/kubernetes/discovery/kubernetes.t:
##########
@@ -286,3 +294,5 @@ GET /compare
Content-type: application/json
--- response_body
true
+--- error_log
+failed to get lua_shared_dict
Review Comment:
But providing a correct conf would be better, instead of using a wrong conf and ignoring the error.
--
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