can read config from cli / env / toml & docker for arm64 & support docker run --rm ceresdb --help #1015
Conversation
xxaier
commented
Jun 21, 2023
xxaier
commented
- docker for arm64
- support docker run --rm ceresdb --help
- can read config from cli / env / toml
2. support docker run --rm ceresdb --help 3. can read config from cli / env / toml
|
|
| @@ -58,15 +58,33 @@ fn main() { | |||
| .takes_value(true) | |||
| .help("Set configuration file, eg: \"/path/server.toml\""), | |||
| ) | |||
| .arg( | |||
There was a problem hiding this comment.
This method seems awkward for me, doesn't the original issue is to support env vars to set config?
There was a problem hiding this comment.
it support both env and cli
see the first image line2
There was a problem hiding this comment.
Could you use existing method to support env? like
https://github.com/CeresDB/ceresdb/blob/8cb5b111854732d65f83df138c1c9b3465dd6c39/src/bin/ceresdb-server.rs#L71
There was a problem hiding this comment.
这个环境变量的配置方式是参考 rust config库的配置方式 参见 https://blog.logrocket.com/configuration-management-in-rust-web-services/
比如 qdrant 就是用的这种配置方式，如下图
https://github.com/qdrant/qdrant
这个配置方式的优点是，不需要修改原来的代码且通用，不然每个地方都要加一个判断，很麻烦
There was a problem hiding this comment.
You use cli_env_toml to do the heavy task, and this is a very new crate, so I'm concern with its stability.
Also, although set config one by one may seems dumb at first, but it make code simple and easy to read, so I suggest you use original style to add env support.
I think following 4 envs are enough for your case.:
CERESDB_BIND_ADDR
CERESDB_HTTP_PORT
CERESDB_MYSQL_PORT
CERESDB_GRPC_PORT
| with: | ||
| platforms: linux/amd64,linux/arm64 |
There was a problem hiding this comment.
Have you tried build on linux/arm64? We haven't fully tested on it, so I'm afraid something maybe wrong.
There was a problem hiding this comment.
How about apply it only in nightly image?
If everything goes fine for some period, we can apply it to production image.
| @@ -58,15 +58,33 @@ fn main() { | |||
| .takes_value(true) | |||
| .help("Set configuration file, eg: \"/path/server.toml\""), | |||
| ) | |||
| .arg( | |||
There was a problem hiding this comment.
Could you use existing method to support env? like
https://github.com/CeresDB/ceresdb/blob/8cb5b111854732d65f83df138c1c9b3465dd6c39/src/bin/ceresdb-server.rs#L71
|
@xxaier Hi, do you have free time to push this PR forward? If not, I can do some minor changes based on your branch, Thanks. |
|
Inactive for too long. If any updates, feel free to reopen it. |
