fix: validation during merging node_listen configuration #4881
Conversation
Signed-off-by: wayne-cheng <[email protected]>
|
OK, Please retest. @spacewander |
Signed-off-by: wayne-cheng <[email protected]>
wayne-cheng
force-pushed
the
fix-merge-conf
branch
from
5aa13ae to
8a69e06
Compare
|
|
||
| echo ' | ||
| apisix: | ||
| node_listen: |
There was a problem hiding this comment.
Could we keep the table in conf/config-default.yaml and use number in config.yaml? Better to avoid touching config-default.yaml in the test.
There was a problem hiding this comment.
The previous existing test cases which use number in config.yaml have achieved this goal you said.
Take an example from the file t/cli/test_main.sh:
echo "
apisix:
node_listen:
- 9080
- 9081
- 9082
ssl:
listen_port:
- 9443
- 9444
- 9445
" > conf/config.yaml
make initThis test that I added in t/cli/test_validate_config.sh is used to test the case that the config-default.yaml use number. In fact, the config-default.yaml hasn’t changed because the test case exec git chekout conf/config-default.yaml in the end.
What do you think? @spacewander
spacewander
changed the title
apisix/cli/file.lua
Outdated
| return true | ||
| end | ||
|
|
||
| if path == "apisix->ssl->listen" and type_val == "number" then |
There was a problem hiding this comment.
what apisix->ssl->listen it is?
There was a problem hiding this comment.
@tzssangglass The config apisix.ssl.listen will be used in another PR (#4856). This will not affect the current code, do I remove it?
There was a problem hiding this comment.
I think remove it is better and add it to the PRs that need it.
There was a problem hiding this comment.
ok, I have removed it, please retest. @tzssangglass
| if (ppath == "nginx_config" or str_sub(ppath, 1, 14) == "nginx_config->") and | ||
| (type_val == "number" or type_val == "string") | ||
| then | ||
| local path = ppath == "" and key or ppath .. "->" .. key |
There was a problem hiding this comment.
can we use JSON schema to check it?
There was a problem hiding this comment.
@membphis The old code has been the schema like ppath .. "->" .. key，and it used to print log. If change this to JSON，many test cases could be affected.
Signed-off-by: wayne-cheng <[email protected]>
Signed-off-by: wayne-cheng [email protected]
This PR is used to solve a bug found in another PR (#4856).
When the conifg
apisix.node_listeis set to a array in theconifg-default.yaml, but the user'sconfig.yamluse the type number,a error will occur.
More detail, Please see #4856 (comment)
PTAL @spacewander
You can use the existing test cases for testing, since the vaule type of the config
apisix.node_listenin theconfig-default.yamlis changed to the type array.Pre-submission checklist: