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/20 02:55:51 UTC
[GitHub] [apisix] monkeyDluffy6017 opened a new pull request, #7946: test: fix config error in test cases
monkeyDluffy6017 opened a new pull request, #7946:
URL: https://github.com/apache/apisix/pull/7946
### Description
Some yaml config in test cases are incorrect because we don't handle the result of schema validatation.
### 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
- [ ] I have added tests corresponding to this change
- [ ] 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)
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r975101697
##########
apisix/core/config_local.lua:
##########
@@ -65,7 +65,10 @@ function _M.local_conf(force)
end
-- fill the default value by the schema
- schema.validate(default_conf)
+ local ok, err = schema.validate(default_conf)
+ if not ok then
+ ngx.log(ngx.ERR, "======================== levy, err: ", err)
Review Comment:
It will be changed to error(), I just want to find out how many test cases will fail and fix 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] zhixiongdu027 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
zhixiongdu027 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974868762
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
maybe we should find another way
@spacewander @monkeyDluffy6017
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974911601
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
Sure, i will check this 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r975101697
##########
apisix/core/config_local.lua:
##########
@@ -65,7 +65,10 @@ function _M.local_conf(force)
end
-- fill the default value by the schema
- schema.validate(default_conf)
+ local ok, err = schema.validate(default_conf)
+ if not ok then
+ ngx.log(ngx.ERR, "======================== levy, err: ", err)
Review Comment:
It will be changed to error() after finding out how many test cases will fail because yaml config.
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r979500889
##########
apisix/core/config_local.lua:
##########
@@ -65,7 +65,10 @@ function _M.local_conf(force)
end
-- fill the default value by the schema
- schema.validate(default_conf)
+ local ok, err = schema.validate(default_conf)
+ if not ok then
+ ngx.log(ngx.ERR, "======================== levy, err: ", err)
Review Comment:
Done
--
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 #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974908599
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
> The APISIX will fail before modify the schema, maybe we should figure out another way to reduce the interval.
@monkeyDluffy6017
Interesting. The modification is done in the init_by_lua phase. It is strange that the schema validation happens before it. Could you dig deeper into 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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974859215
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
The APISIX will fail before modify the schema, maybe we should figure out another way to reduce the interval.
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r979504595
##########
t/APISIX.pm:
##########
@@ -493,6 +493,11 @@ _EOC_
$block->set_value("main_config", $main_config);
+ # The new directive is introduced here to modify the upper and
+ # lower limits of the schema before apisix validate in require("apisix")
+ # Todo: merge extra_init_by_lua_start and extra_init_by_lua
Review Comment:
https://github.com/apache/apisix/discussions/7988
--
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 diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r975098868
##########
apisix/core/config_local.lua:
##########
@@ -65,7 +65,10 @@ function _M.local_conf(force)
end
-- fill the default value by the schema
- schema.validate(default_conf)
+ local ok, err = schema.validate(default_conf)
+ if not ok then
+ ngx.log(ngx.ERR, "======================== levy, err: ", err)
Review Comment:
levy?
--
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] zhixiongdu027 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
zhixiongdu027 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974853813
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
Please see [link](https://github.com/apache/apisix/pull/6599#discussion_r827587517)
> "full_fetch_interval: 3"
> "incremental_fetch_interval: 1"
I think we should keep it as it is
--
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 #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r979398089
##########
t/APISIX.pm:
##########
@@ -502,6 +503,8 @@ _EOC_
require "resty.core"
+ $extra_init_by_lua_start
Review Comment:
Ping @monkeyDluffy6017
--
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] monkeyDluffy6017 commented on pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on PR #7946:
URL: https://github.com/apache/apisix/pull/7946#issuecomment-1257363332
test
--
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 #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r979501563
##########
t/APISIX.pm:
##########
@@ -493,6 +493,11 @@ _EOC_
$block->set_value("main_config", $main_config);
+ # The new directive is introduced here to modify the upper and
Review Comment:
We can modify the schema, not only the upper and lower limits
##########
t/APISIX.pm:
##########
@@ -493,6 +493,11 @@ _EOC_
$block->set_value("main_config", $main_config);
+ # The new directive is introduced here to modify the upper and
+ # lower limits of the schema before apisix validate in require("apisix")
+ # Todo: merge extra_init_by_lua_start and extra_init_by_lua
Review Comment:
Maybe we should also create a new issue for this tech debt.
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974863364
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
https://github.com/apache/apisix/blob/master/apisix/core/config_local.lua#L68
```
-- fill the default value by the schema
schema.validate(default_conf)
```
We didn't handle the result of schema validation before, that's why your solution work, we need to throw `error` now
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r975101697
##########
apisix/core/config_local.lua:
##########
@@ -65,7 +65,10 @@ function _M.local_conf(force)
end
-- fill the default value by the schema
- schema.validate(default_conf)
+ local ok, err = schema.validate(default_conf)
+ if not ok then
+ ngx.log(ngx.ERR, "======================== levy, err: ", err)
Review Comment:
It will be changed to error(), I just want to find out how many test cases will failed and fix 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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974859215
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
The APISIX will fail before modify the schema, maybe we should figure out other way to reduce the interval.
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974863364
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
https://github.com/apache/apisix/blob/master/apisix/core/config_local.lua#L68
```
-- fill the default value by the schema
schema.validate(default_conf)
```
We didn't handle the result of schema validation before, that's why your solution work, we need to throw error now
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r974863364
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
```
-- fill the default value by the schema
schema.validate(default_conf)
```
We didn't handle the result of schema validation before, that's why your solution work, we need to throw error now
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r979504433
##########
t/APISIX.pm:
##########
@@ -493,6 +493,11 @@ _EOC_
$block->set_value("main_config", $main_config);
+ # The new directive is introduced here to modify the upper and
+ # lower limits of the schema before apisix validate in require("apisix")
+ # Todo: merge extra_init_by_lua_start and extra_init_by_lua
Review Comment:
https://github.com/apache/apisix/discussions/7988
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r975102127
##########
apisix/core/config_local.lua:
##########
@@ -65,7 +65,10 @@ function _M.local_conf(force)
end
-- fill the default value by the schema
- schema.validate(default_conf)
+ local ok, err = schema.validate(default_conf)
+ if not ok then
+ ngx.log(ngx.ERR, "======================== levy, err: ", err)
Review Comment:
I have changed this pr to 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.
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 #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r978776558
##########
t/APISIX.pm:
##########
@@ -502,6 +503,8 @@ _EOC_
require "resty.core"
+ $extra_init_by_lua_start
Review Comment:
Could we just move $extra_init_by_lua to this place?
--
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 #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7946:
URL: https://github.com/apache/apisix/pull/7946
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r979498437
##########
t/tars/discovery/tars.t:
##########
@@ -41,8 +41,8 @@ discovery:
database: db_tars
user: root
password: tars2022
- full_fetch_interval: 3
- incremental_fetch_interval: 1
+ full_fetch_interval: 90
Review Comment:
The modification should be put before apisix = require("apisix")，the validation is called in that.
##########
t/APISIX.pm:
##########
@@ -502,6 +503,8 @@ _EOC_
require "resty.core"
+ $extra_init_by_lua_start
Review Comment:
What do you ping me for?
##########
t/APISIX.pm:
##########
@@ -502,6 +503,8 @@ _EOC_
require "resty.core"
+ $extra_init_by_lua_start
Review Comment:
Some code chunk in test cases need to be placed after apisix = require("apisix"), so we cannot just move $extra_init_by_lua
##########
t/APISIX.pm:
##########
@@ -502,6 +503,8 @@ _EOC_
require "resty.core"
+ $extra_init_by_lua_start
Review Comment:
Done
##########
t/APISIX.pm:
##########
@@ -502,6 +503,8 @@ _EOC_
require "resty.core"
+ $extra_init_by_lua_start
Review Comment:
Some code chunk in test cases need to be placed after `apisix = require("apisix")`, so we cannot just move $extra_init_by_lua
--
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] monkeyDluffy6017 commented on a diff in pull request #7946: test: fix config error in test cases
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7946:
URL: https://github.com/apache/apisix/pull/7946#discussion_r979505882
##########
t/APISIX.pm:
##########
@@ -493,6 +493,11 @@ _EOC_
$block->set_value("main_config", $main_config);
+ # The new directive is introduced here to modify the upper and
Review Comment:
Done
--
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