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 2020/08/22 13:24:39 UTC
[GitHub] [apisix] Yiyiyimu opened a new pull request #2101: feature: customed config.yaml when apisix start
Yiyiyimu opened a new pull request #2101:
URL: https://github.com/apache/apisix/pull/2101
### What this PR does / why we need it:
fix #2034 : allow user to specify the `conf/config.yaml` when starting the APISIX project
Copy customer config.yaml to default location for compatibility.
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r559275379
##########
File path: apisix/cli/ops.lua
##########
@@ -390,6 +392,21 @@ local function start(env, ...)
end
end
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customized config.yaml")
+ local args = parser:parse()
+ local customized_yaml = args["config"]
+
+ profile.apisix_home = env.apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customized_yaml then
+ execute("mv " .. local_conf_path .. " " .. local_conf_path .. ".bak")
Review comment:
Sure, working on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-757672849
@Yiyiyimu need you to reslove it again :(
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475309509
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "argparse = 0.7.1-1",
Review comment:
MIT License, and it is currently maintained by luarocks community.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r565873865
##########
File path: apisix/cli/etcd.lua
##########
@@ -246,9 +246,6 @@ function _M.init(env, show_output)
break
end
- if show_output then
Review comment:
It would get conflict with the arguments got from `argparse`. As we discussed in #2624, I will implement the debug mode also with argument parsing in.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-755124834
Hi @moonming conflict fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r493136275
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
Good idea! Update to `ln`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r561775948
##########
File path: .github/workflows/lint.yml
##########
@@ -1,6 +1,6 @@
name: ❄️ Lint
-on: [pull_request]
+on: [push, pull_request]
Review comment:
why do we need to change it in this PR?
one PR for one thing
##########
File path: .travis/linux_openresty_common_runner.sh
##########
@@ -108,7 +108,7 @@ script() {
./bin/apisix init_etcd
./bin/apisix start
- #start again --> fial
+ #start again --> fail
Review comment:
we should fix it in a new PR.
one PR for one thing
##########
File path: Makefile
##########
@@ -98,7 +98,7 @@ endif
### stop: Stop the apisix server
.PHONY: stop
stop: default
- $(OR_EXEC) -p $$PWD/ -c $$PWD/conf/nginx.conf -s stop
+ ./bin/apisix stop
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-720967373
@Yiyiyimu
Do you have time to solve the conflicts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r555039722
##########
File path: apisix/cli/ops.lua
##########
@@ -390,6 +392,21 @@ local function start(env, ...)
end
end
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customized config.yaml")
+ local args = parser:parse()
+ local customized_yaml = args["config"]
+
+ profile.apisix_home = env.apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customized_yaml then
+ execute("mv " .. local_conf_path .. " " .. local_conf_path .. ".bak")
Review comment:
and we need the test cases about this step
##########
File path: apisix/cli/ops.lua
##########
@@ -390,6 +392,21 @@ local function start(env, ...)
end
end
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customized config.yaml")
+ local args = parser:parse()
+ local customized_yaml = args["config"]
+
+ profile.apisix_home = env.apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customized_yaml then
+ execute("mv " .. local_conf_path .. " " .. local_conf_path .. ".bak")
Review comment:
we need to revert the file `con/config.yaml.bak` when call `apisix stop`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r484305423
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
If users start two APISIX instances, should it create two nginx.conf? If it creates one nginx.conf, we need to read nginx.conf to get the first location of config.yaml, but it seems not so common to read from nginx.conf.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-769547213
@Yiyiyimu merged, many thx
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r517130471
##########
File path: bin/apisix
##########
@@ -668,6 +668,23 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
Review comment:
[For envoy](https://www.envoyproxy.io/docs/envoy/latest/operations/cli#cmdoption-c), `-c` is the abbr of `--config-path`. Or I could change it to `config-path`, if needed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475091197
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -204,3 +204,26 @@ fi
sed -i 's/worker_processes: 2/worker_processes: auto/' conf/config.yaml
echo "passed: worker_processes number is configurable"
+
+# check customed config.yaml is copied.
+
+git checkout conf/config.yaml
+
+echo "
+nginx_config:
+ worker_processes: 2
+" > conf/customed_config.yaml
+
+make default
+./bin/apisix init
+./bin/apisix init_etcd
+./bin/apisix start -c conf/customed_config.yaml
Review comment:
test needs to improve. call `apisix start` here would failed.
**Need suggestions!**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r565843688
##########
File path: doc/architecture-design.md
##########
@@ -643,13 +645,13 @@ Enable advanced debug mode by modifying the configuration in `conf/debug.yaml` f
The checker would judge whether the file data changed according to the last modification time of the file. If there has any change, reload it. If there was no change, skip this check. So it's hot reload for enabling or disabling advanced debug mode.
-|Key|Optional|Description|Default|
-|----|-----|---------|---|
-|hook_conf.enable|required|Enable/Disable hook debug trace. Target module function's input arguments or returned value would be printed once this option is enabled.|false|
-|hook_conf.name|required|The module list name of hook which has enabled debug trace||
-|hook_conf.log_level|required|Logging levels for input arguments & returned value|warn|
-|hook_conf.is_print_input_args|required|Enable/Disable input arguments print|true|
-|hook_conf.is_print_return_value|required|Enable/Disable returned value print|true|
+| Key | Optional | Description | Default |
+| ------------------------------- | -------- | ----------------------------------------------------------------------------------------------------------------------------------------- | ------- |
+| hook_conf.enable | required | Enable/Disable hook debug trace. Target module function's input arguments or returned value would be printed once this option is enabled. | false |
+| hook_conf.name | required | The module list name of hook which has enabled debug trace | |
+| hook_conf.log_level | required | Logging levels for input arguments & returned value | warn |
+| hook_conf.is_print_input_args | required | Enable/Disable input arguments print | true |
+| hook_conf.is_print_return_value | required | Enable/Disable returned value print | true |
Review comment:
I think it would be better to add `.` after the sentence in the `Description` field.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r484328359
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
Also, I think it would be too complicated when we save the location of customized config.yaml in nginx.conf, since it would be pass this one parameter through quite a lot functions to config_local.yaml to read.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r517053138
##########
File path: bin/apisix
##########
@@ -668,6 +668,23 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
Review comment:
Should be `customized`? Please change other similar places
##########
File path: doc/architecture-design.md
##########
@@ -45,15 +45,17 @@
## APISIX Config
-For example, set the default listening port of APISIX to 8000, and keep other configurations as default. The configuration in `conf/config.yaml` should be like this:
+There are two methods to configure APISIX: directly change `conf/config.yaml`, or add file path argument when start APISIX like `apisix start -c /opts/apisix/config.yaml`
Review comment:
Better to mention `--config` option too.
##########
File path: bin/apisix
##########
@@ -631,7 +631,7 @@ local function init_etcd(show_output)
break
end
- if show_output then
+ if show_output and show_output ~= "-c" then
Review comment:
It would be better to pass the argument explicitly. Filter out all the other options is error-prone. For example, is `--config` handled?
##########
File path: doc/architecture-design.md
##########
@@ -45,15 +45,17 @@
## APISIX Config
-For example, set the default listening port of APISIX to 8000, and keep other configurations as default. The configuration in `conf/config.yaml` should be like this:
+There are two methods to configure APISIX: directly change `conf/config.yaml`, or add file path argument when start APISIX like `apisix start -c /opts/apisix/config.yaml`
+
+For example, set the default listening port of APISIX to 8000, and keep other configurations as default. The configuration in `config.yaml` should be like this:
```yaml
apisix:
node_listen: 8000 # APISIX listening port
```
Set the default listening port of APISIX to 8000, set the `etcd` address to `http://foo:2379`,
-and keep other configurations as default. The configuration in `conf/config.yaml` should be like this:
Review comment:
Why remove the `conf/`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475217758
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
Since the further operation after `apisix start` would still use this customized yaml, there is need to save the location somewhere for further calling. And let's say save the location to config.yaml, would that be more elegant?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r485675296
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
another question, if the user updates the `config.yaml`, the APISIX instance is no way to load the latest `config.yaml`.
use `ln` is better than `cp`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-678662273
> @Yiyiyimu and we need to update the document about this new feature
Sure I just added the relevant document.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475276226
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
We should allow users to start two APISIX instances, this way is wrong.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r484305423
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
If users start two APISIX instances, should it create two nginx.conf? If it creates one nginx.conf, we need to read nginx.conf to get the first location of config.yaml, ~~but it seems not so common to read from nginx.conf~~, is that the correct method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r493136275
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
Good idea! Update to `ln`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475092789
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -204,3 +204,26 @@ fi
sed -i 's/worker_processes: 2/worker_processes: auto/' conf/config.yaml
echo "passed: worker_processes number is configurable"
+
+# check customed config.yaml is copied.
+
+git checkout conf/config.yaml
+
+echo "
+nginx_config:
+ worker_processes: 2
+" > conf/customed_config.yaml
+
+make default
+./bin/apisix init
+./bin/apisix init_etcd
+./bin/apisix start -c conf/customed_config.yaml
Review comment:
run the APISIX server, and confirm it load the right config file, eg `conf/customed_config.yaml`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r484058538
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
we can pass this option by `nginx.conf`. we can follow this way: https://github.com/apache/apisix/blob/master/bin/apisix#L279
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r561602243
##########
File path: Makefile
##########
@@ -98,7 +98,7 @@ endif
### stop: Stop the apisix server
.PHONY: stop
stop: default
- $(OR_EXEC) -p $$PWD/ -c $$PWD/conf/nginx.conf -s stop
+ ./bin/apisix stop
Review comment:
sync to init/run
##########
File path: .travis/apisix_cli_test/test_main.sh
##########
@@ -500,6 +500,43 @@ fi
sed -i 's/worker_processes: 2/worker_processes: auto/' conf/config.yaml
echo "passed: worker_processes number is configurable"
+# check customized config.yaml is copied and reverted.
+
+git checkout conf/config.yaml
+
+echo "
+apisix:
+ admin_api_mtls:
+ admin_ssl_cert: '../t/certs/apisix_admin_ssl.crt'
+ admin_ssl_cert_key: '../t/certs/apisix_admin_ssl.key'
+ port_admin: 9180
+ https_admin: true
+" > conf/customized_config.yaml
+
+make init
+
+./bin/apisix start -c conf/customized_config.yaml
+
+code=$(curl -k -i -m 20 -o /dev/null -s -w %{http_code} https://127.0.0.1:9180/apisix/admin/routes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1')
+if [ ! $code -eq 200 ]; then
+ echo "failed: customized config.yaml copied failed"
+ exit 1
+fi
+# to revert the customized config and start a new one
+make stop
+
+./bin/apisix start
+
+code=$(curl -k -i -m 20 -o /dev/null -s -w %{http_code} https://127.0.0.1:9180/apisix/admin/routes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1')
Review comment:
This line would return code `0` since connection refused, so it would be trapped and `clean_up` will execute
##########
File path: .travis/apisix_cli_test/test_main.sh
##########
@@ -500,6 +500,43 @@ fi
sed -i 's/worker_processes: 2/worker_processes: auto/' conf/config.yaml
echo "passed: worker_processes number is configurable"
+# check customized config.yaml is copied and reverted.
+
+git checkout conf/config.yaml
+
+echo "
+apisix:
+ admin_api_mtls:
+ admin_ssl_cert: '../t/certs/apisix_admin_ssl.crt'
+ admin_ssl_cert_key: '../t/certs/apisix_admin_ssl.key'
+ port_admin: 9180
+ https_admin: true
+" > conf/customized_config.yaml
+
+make init
+
+./bin/apisix start -c conf/customized_config.yaml
+
+code=$(curl -k -i -m 20 -o /dev/null -s -w %{http_code} https://127.0.0.1:9180/apisix/admin/routes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1')
+if [ ! $code -eq 200 ]; then
+ echo "failed: customized config.yaml copied failed"
+ exit 1
+fi
+# to revert the customized config and start a new one
+make stop
+
+./bin/apisix start
+
+code=$(curl -k -i -m 20 -o /dev/null -s -w %{http_code} https://127.0.0.1:9180/apisix/admin/routes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1')
Review comment:
Maybe I could compare if after `make stop`, is `config.yaml` still the same with the original one
##########
File path: apisix/cli/ops.lua
##########
@@ -390,6 +392,21 @@ local function start(env, ...)
end
end
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customized config.yaml")
+ local args = parser:parse()
+ local customized_yaml = args["config"]
+
+ profile.apisix_home = env.apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customized_yaml then
+ execute("mv " .. local_conf_path .. " " .. local_conf_path .. ".bak")
Review comment:
PTAL
##########
File path: .github/workflows/lint.yml
##########
@@ -1,6 +1,6 @@
name: ❄️ Lint
-on: [pull_request]
+on: [push, pull_request]
Review comment:
Thanks! Fix the first two unrelated changes.
The last one is what this PR need, otherwise `make stop` could not revert the `config.yaml.bak`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r555004103
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -59,6 +59,7 @@ dependencies = {
"lua-resty-expr = 1.0.0",
"graphql = 0.0.2",
"argparse = 0.7.1-1",
+ "luasocket = 3.0rc1-2",
Review comment:
I think we should use a `table` version
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r561602243
##########
File path: Makefile
##########
@@ -98,7 +98,7 @@ endif
### stop: Stop the apisix server
.PHONY: stop
stop: default
- $(OR_EXEC) -p $$PWD/ -c $$PWD/conf/nginx.conf -s stop
+ ./bin/apisix stop
Review comment:
sync to init/run
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r561775948
##########
File path: .github/workflows/lint.yml
##########
@@ -1,6 +1,6 @@
name: ❄️ Lint
-on: [pull_request]
+on: [push, pull_request]
Review comment:
why do we need to change it in this PR?
one PR for one thing
##########
File path: .travis/linux_openresty_common_runner.sh
##########
@@ -108,7 +108,7 @@ script() {
./bin/apisix init_etcd
./bin/apisix start
- #start again --> fial
+ #start again --> fail
Review comment:
we should fix it in a new PR.
one PR for one thing
##########
File path: Makefile
##########
@@ -98,7 +98,7 @@ endif
### stop: Stop the apisix server
.PHONY: stop
stop: default
- $(OR_EXEC) -p $$PWD/ -c $$PWD/conf/nginx.conf -s stop
+ ./bin/apisix stop
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r565883045
##########
File path: apisix/cli/ops.lua
##########
@@ -406,6 +408,21 @@ local function start(env, ...)
end
end
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customized config.yaml")
+ local args = parser:parse()
+ local customized_yaml = args["config"]
+
+ profile.apisix_home = env.apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customized_yaml then
+ execute("mv " .. local_conf_path .. " " .. local_conf_path .. ".bak")
+ execute("ln " .. customized_yaml .. " " .. local_conf_path)
Review comment:
Fixed. Thanks!
##########
File path: doc/architecture-design.md
##########
@@ -643,13 +645,13 @@ Enable advanced debug mode by modifying the configuration in `conf/debug.yaml` f
The checker would judge whether the file data changed according to the last modification time of the file. If there has any change, reload it. If there was no change, skip this check. So it's hot reload for enabling or disabling advanced debug mode.
-|Key|Optional|Description|Default|
-|----|-----|---------|---|
-|hook_conf.enable|required|Enable/Disable hook debug trace. Target module function's input arguments or returned value would be printed once this option is enabled.|false|
-|hook_conf.name|required|The module list name of hook which has enabled debug trace||
-|hook_conf.log_level|required|Logging levels for input arguments & returned value|warn|
-|hook_conf.is_print_input_args|required|Enable/Disable input arguments print|true|
-|hook_conf.is_print_return_value|required|Enable/Disable returned value print|true|
+| Key | Optional | Description | Default |
+| ------------------------------- | -------- | ----------------------------------------------------------------------------------------------------------------------------------------- | ------- |
+| hook_conf.enable | required | Enable/Disable hook debug trace. Target module function's input arguments or returned value would be printed once this option is enabled. | false |
+| hook_conf.name | required | The module list name of hook which has enabled debug trace | |
+| hook_conf.log_level | required | Logging levels for input arguments & returned value | warn |
+| hook_conf.is_print_input_args | required | Enable/Disable input arguments print | true |
+| hook_conf.is_print_return_value | required | Enable/Disable returned value print | true |
Review comment:
Fixed. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r517136760
##########
File path: doc/architecture-design.md
##########
@@ -45,15 +45,17 @@
## APISIX Config
-For example, set the default listening port of APISIX to 8000, and keep other configurations as default. The configuration in `conf/config.yaml` should be like this:
+There are two methods to configure APISIX: directly change `conf/config.yaml`, or add file path argument when start APISIX like `apisix start -c /opts/apisix/config.yaml`
+
+For example, set the default listening port of APISIX to 8000, and keep other configurations as default. The configuration in `config.yaml` should be like this:
```yaml
apisix:
node_listen: 8000 # APISIX listening port
```
Set the default listening port of APISIX to 8000, set the `etcd` address to `http://foo:2379`,
-and keep other configurations as default. The configuration in `conf/config.yaml` should be like this:
Review comment:
I think since config.yaml in `conf` folder is not the only choice, we could remove this
Do you think removing it will make users could not find the config file?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis merged pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #2101:
URL: https://github.com/apache/apisix/pull/2101
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nic-chen edited a comment on pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
nic-chen edited a comment on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-678714864
@Yiyiyimu not just for `apisix start`, we need to handle here too:
https://github.com/apache/apisix/blob/master/apisix/core/config_local.lua#L30
https://github.com/apache/apisix/blob/master/apisix/core/profile.lua#L26
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475107379
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -204,3 +204,26 @@ fi
sed -i 's/worker_processes: 2/worker_processes: auto/' conf/config.yaml
echo "passed: worker_processes number is configurable"
+
+# check customed config.yaml is copied.
+
+git checkout conf/config.yaml
+
+echo "
+nginx_config:
+ worker_processes: 2
+" > conf/customed_config.yaml
+
+make default
+./bin/apisix init
+./bin/apisix init_etcd
+./bin/apisix start -c conf/customed_config.yaml
Review comment:
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r561839068
##########
File path: .github/workflows/lint.yml
##########
@@ -1,6 +1,6 @@
name: ❄️ Lint
-on: [pull_request]
+on: [push, pull_request]
Review comment:
Thanks! Fix the first two unrelated changes.
The last one is what this PR need, otherwise `make stop` could not revert the `config.yaml.bak`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r561629722
##########
File path: .travis/apisix_cli_test/test_main.sh
##########
@@ -500,6 +500,43 @@ fi
sed -i 's/worker_processes: 2/worker_processes: auto/' conf/config.yaml
echo "passed: worker_processes number is configurable"
+# check customized config.yaml is copied and reverted.
+
+git checkout conf/config.yaml
+
+echo "
+apisix:
+ admin_api_mtls:
+ admin_ssl_cert: '../t/certs/apisix_admin_ssl.crt'
+ admin_ssl_cert_key: '../t/certs/apisix_admin_ssl.key'
+ port_admin: 9180
+ https_admin: true
+" > conf/customized_config.yaml
+
+make init
+
+./bin/apisix start -c conf/customized_config.yaml
+
+code=$(curl -k -i -m 20 -o /dev/null -s -w %{http_code} https://127.0.0.1:9180/apisix/admin/routes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1')
+if [ ! $code -eq 200 ]; then
+ echo "failed: customized config.yaml copied failed"
+ exit 1
+fi
+# to revert the customized config and start a new one
+make stop
+
+./bin/apisix start
+
+code=$(curl -k -i -m 20 -o /dev/null -s -w %{http_code} https://127.0.0.1:9180/apisix/admin/routes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1')
Review comment:
Maybe I could compare if after `make stop`, is `config.yaml` still the same with the original one
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r517145994
##########
File path: bin/apisix
##########
@@ -668,6 +668,23 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
Review comment:
Ah thx fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-678719772
> @Yiyiyimu not just for `apisix start`, we need to handle here too:
>
> https://github.com/apache/apisix/blob/master/apisix/core/config_local.lua#L30
> https://github.com/apache/apisix/blob/master/apisix/core/profile.lua#L26
Yes I've thought about it so I directly copied the customed config.yaml to our default location, and we don't need to worry about the other places which interact with config.yaml.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475213246
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
I do not think that is the right way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r484305423
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
If users start two APISIX instances, should it create two nginx.conf? If it creates one nginx.conf, we need to read nginx.conf to get the first location of config.yaml, ~~but it seems not so common to read from nginx.conf~~.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r566531425
##########
File path: apisix/cli/etcd.lua
##########
@@ -246,9 +246,6 @@ function _M.init(env, show_output)
break
end
- if show_output then
Review comment:
Got 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r517139365
##########
File path: bin/apisix
##########
@@ -668,6 +668,23 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
Review comment:
@Yiyiyimu
Actually I mean `customed` should be `customized`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-678714864
@Yiyiyimu not just for `apisix start`, we need to handle here:
https://github.com/apache/apisix/blob/master/apisix/core/profile.lua#L26
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r565855159
##########
File path: apisix/cli/etcd.lua
##########
@@ -246,9 +246,6 @@ function _M.init(env, show_output)
break
end
- if show_output then
Review comment:
Why remove this?
##########
File path: apisix/cli/ops.lua
##########
@@ -406,6 +408,21 @@ local function start(env, ...)
end
end
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customized config.yaml")
+ local args = parser:parse()
+ local customized_yaml = args["config"]
+
+ profile.apisix_home = env.apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customized_yaml then
+ execute("mv " .. local_conf_path .. " " .. local_conf_path .. ".bak")
+ execute("ln " .. customized_yaml .. " " .. local_conf_path)
Review comment:
Use `execute_cmd` in `apisix.cli.utils` .
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r517143200
##########
File path: bin/apisix
##########
@@ -631,7 +631,7 @@ local function init_etcd(show_output)
break
end
- if show_output then
+ if show_output and show_output ~= "-c" then
Review comment:
I think we could remove `show_output`, since it's used to debug init_etcd. I raised an issue in #2624
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475294814
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -52,6 +52,7 @@ dependencies = {
"lua-resty-kafka = 0.07",
"lua-resty-logger-socket = 2.0-0",
"skywalking-nginx-lua-plugin = 1.0-0",
+ "argparse = 0.7.1-1",
Review comment:
what's the license of `argparse`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r555011602
##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -59,6 +59,7 @@ dependencies = {
"lua-resty-expr = 1.0.0",
"graphql = 0.0.2",
"argparse = 0.7.1-1",
+ "luasocket = 3.0rc1-2",
Review comment:
This pkg was imported in #2965, maybe we could fix it in another pr
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-721070458
> Do you have time to solve the conflicts?
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r561629415
##########
File path: .travis/apisix_cli_test/test_main.sh
##########
@@ -500,6 +500,43 @@ fi
sed -i 's/worker_processes: 2/worker_processes: auto/' conf/config.yaml
echo "passed: worker_processes number is configurable"
+# check customized config.yaml is copied and reverted.
+
+git checkout conf/config.yaml
+
+echo "
+apisix:
+ admin_api_mtls:
+ admin_ssl_cert: '../t/certs/apisix_admin_ssl.crt'
+ admin_ssl_cert_key: '../t/certs/apisix_admin_ssl.key'
+ port_admin: 9180
+ https_admin: true
+" > conf/customized_config.yaml
+
+make init
+
+./bin/apisix start -c conf/customized_config.yaml
+
+code=$(curl -k -i -m 20 -o /dev/null -s -w %{http_code} https://127.0.0.1:9180/apisix/admin/routes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1')
+if [ ! $code -eq 200 ]; then
+ echo "failed: customized config.yaml copied failed"
+ exit 1
+fi
+# to revert the customized config and start a new one
+make stop
+
+./bin/apisix start
+
+code=$(curl -k -i -m 20 -o /dev/null -s -w %{http_code} https://127.0.0.1:9180/apisix/admin/routes -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1')
Review comment:
This line would return code `0` since connection refused, so it would be trapped and `clean_up` will execute
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r475309706
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
I got it. I'll change 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-768759933
Hi @membphis PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-678643282
@Yiyiyimu and we need to update the document about this new feature
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feature: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r484328359
##########
File path: bin/apisix
##########
@@ -940,6 +940,22 @@ function _M.start(...)
end
end
+ local argparse = require "argparse"
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customed config.yaml")
+ local args = parser:parse()
+ local customed_yaml = args["config"]
+
+ local profile = require("apisix.core.profile")
+ profile.apisix_home = apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customed_yaml then
+ local cmd = "cp " .. customed_yaml .. " " .. local_conf_path
Review comment:
Also, I think it would be too complicated when we save the location of customized config.yaml in nginx.conf, since it would be too complicated to pass this one parameter through quite a lot functions to config_local.yaml to read.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r517146559
##########
File path: doc/architecture-design.md
##########
@@ -45,15 +45,17 @@
## APISIX Config
-For example, set the default listening port of APISIX to 8000, and keep other configurations as default. The configuration in `conf/config.yaml` should be like this:
+There are two methods to configure APISIX: directly change `conf/config.yaml`, or add file path argument when start APISIX like `apisix start -c /opts/apisix/config.yaml`
Review comment:
fixed thx
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#discussion_r561647161
##########
File path: apisix/cli/ops.lua
##########
@@ -390,6 +392,21 @@ local function start(env, ...)
end
end
+ local parser = argparse()
+ parser:argument("_", "Placeholder")
+ parser:option("-c --config", "location of customized config.yaml")
+ local args = parser:parse()
+ local customized_yaml = args["config"]
+
+ profile.apisix_home = env.apisix_home .. "/"
+ local local_conf_path = profile:yaml_path("config")
+
+ if customized_yaml then
+ execute("mv " .. local_conf_path .. " " .. local_conf_path .. ".bak")
Review comment:
PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2101: feat: customed config.yaml when apisix start
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2101:
URL: https://github.com/apache/apisix/pull/2101#issuecomment-754300347
ping @Yiyiyimu
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org