Rework usage of Krustlet config to allow passing parameters on the command line #36
Conversation
Pulled out documentation strings into separate files, which allows editing them as proper adoc files with preview, instead of string literals in Rust code. This makes writing documentation _a lot_ easier.
|
Please feel free to take a look, I expect that I'll find some minor things to fix in this still, but the overall structure and code should be done. |
Added parsing of bootstrapfile from command line.
src/agentconfig.rs
Outdated
| default: Some("/etc/stackable/agent/plugins"), | ||
| required: false, | ||
| takes_argument: true, | ||
| help: "The directory to observe for new sockets to appear which can be used to communicate with CSI plugins.", |
There was a problem hiding this comment.
We are not using this, no. It was more a matter of making it configurable "just in case" - I am happy to just default this to something relative to the data dir so that if for some reason it gets created it is out of the way.
There was a problem hiding this comment.
Just in case.... CSI plugins? I believe it confuses more than it helps to be honest.
src/agentconfig.rs
Outdated
| @@ -228,6 +289,51 @@ impl Configurable for AgentConfig { | |||
| }; | |||
| info!("Selected {} as local address to listen on.", final_ip); | |||
|
|
|||
| // Parse data directory from values | |||
| let final_data_dir = if let Ok(data_dir) = | |||
There was a problem hiding this comment.
If we add one more it makes sense to move it into a function. This is a 1:1 copy of the stuff below
There was a problem hiding this comment.
I considered it, but opted for "doesn't have to look nice at this stage" - which incidentally you keep telling me ;)
There was a problem hiding this comment.
I pulled the code into a fn and refactored it a bit.
src/agentconfig.rs
Outdated
| }) | ||
| } else { | ||
| // Should not happen due to default value being assigned to the parameter | ||
| PathBuf::from("") |
There was a problem hiding this comment.
If this should not happen wouldn't it make more sense to cause a panic if it does? Especially as this is on startup it'd catch programming errors.
So basically change the if let to something like ...unwrap()
I haven't fully thought it through but should work, no?
There was a problem hiding this comment.
I've extracted this code into a function and refactored it a bit.
src/agentconfig.rs
Outdated
| u16::from_str(&server_port) | ||
| .unwrap_or_else(|_| panic!("Error parsing webserver port from [{}], aborting.")) | ||
| } else { | ||
| // This should not be necessary, as the default should be provided by clap, |
There was a problem hiding this comment.
Same as above. If it shouldn't happen let's panic?
There was a problem hiding this comment.
I've refactored the code and moved this bit out into a fn.
|
This should also update the readme once #35 is merged |
|
And there are a few clippy warnings. I haven't mentioned them separately. |
Me neither, I'm open to suggestions, but this was the best compromise I could come up with. |
…ted collecting all parse errors before panicing.
I've fixed the ones that are not about naming - those have been around for a while, I'll fix those in a separate PR. |
# Conflicts: # Cargo.lock # Cargo.toml
src/main.rs
Outdated
| addr: agent_config.server_ip_address.clone(), | ||
| port: agent_config.server_port, | ||
| cert_file: agent_config.server_cert_file.unwrap_or(Default::default()), | ||
| private_key_file: agent_config.server_key_file.unwrap_or(Default::default()), |
There was a problem hiding this comment.
| private_key_file: agent_config.server_key_file.unwrap_or(Default::default()), | |
| private_key_file: agent_config.server_key_file.unwrap_or_default(), |
src/main.rs
Outdated
| let server_config = ServerConfig { | ||
| addr: agent_config.server_ip_address.clone(), | ||
| port: agent_config.server_port, | ||
| cert_file: agent_config.server_cert_file.unwrap_or(Default::default()), |
There was a problem hiding this comment.
| cert_file: agent_config.server_cert_file.unwrap_or(Default::default()), | |
| cert_file: agent_config.server_cert_file.unwrap_or_default(), |
Fixes #34