Repository navigation
Add ability to merge config from an external file. - #278
Conversation
|
Oh yeah one note: it wasnt clear to me what purpose |
| pub async fn merge_main(opt: Opt) -> Result<(), anyhow::Error> { | ||
| let pusher_config = MergeConfig::parse_file(&opt.merger_config_path)?; | ||
|
|
||
| let external_execution_config = match &opt.external_config_path { |
There was a problem hiding this comment.
we can't use .map here or something?
ahh maybe not because of the ? man... ? is really a very poor man's traverse.
| pub async fn merge( | ||
| merge_config: &super::merge_config::MergeConfig, | ||
| relative_search_path: &Option<PathBuf>, | ||
| external_execution_config: &Option<ExecutionConfig>, |
There was a problem hiding this comment.
seems like this could be a &Vector<ExecutionConfig> and just iterate through updating?
There was a problem hiding this comment.
Yeah, re: the comment on your other PR, if we're going to be able to have multiple file configs, this will need to be a vector.
there is an ExecutionConfig buried in the MergeConfig passed in here, and that one needs to be the last merge, so its fine to leave it as-is.
|
@johnynek I updated this to take in multiple external configs, want to take another look? |
There was a problem hiding this comment.
looks good, I just had a suggestion on removing a .clone
| let mut configs = Vec::new(); | ||
| for p in paths { | ||
| let json_str = fs::read_to_string(&p)?; | ||
| let config = serde_json::from_str(&json_str)?; |
There was a problem hiding this comment.
can we use https://docs.rs/serde_json/latest/serde_json/fn.from_reader.html to read the file directly rather than read string then string to json?
| merge_config: &super::merge_config::MergeConfig, | ||
| relative_search_path: &Option<PathBuf>, | ||
| external_execution_config: &Option<ExecutionConfig>, | ||
| external_execution_configs: &Vec<ExecutionConfig>, |
There was a problem hiding this comment.
nit: I if you remove the &Vec here and just do Vec<ExecutionConfig> we can avoid the clone below. Seems like the caller is the last to use this config.
| for config in external_execution_configs { | ||
| let mut external_base_cfg = ConfigDelta::default(); | ||
| external_base_cfg.config = external_execution_config.clone(); | ||
| external_base_cfg.config = Some(config.clone()); |
There was a problem hiding this comment.
I think we can remove this .clone if we make external_execution_configs not a reference.
There was a problem hiding this comment.
ah this was a nice catch
We'd like to add the ability to load an external file and pass that as a config, more details here: bazeltools/rules_minidock#8
This PR implements this in the merger app. Rules-based config will always take precedence over the external config file in the event of a conflict.
I moved a bunch of stuff around in the app so that I could add an integration test suite.