Add --patch option for all commands that takes an experiment #5721
Conversation
| @@ -412,4 +475,66 @@ mod tests { | |||
| ); | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_patch_value() -> Result<()> { | |||
| pub(crate) fn patch_value(this: &mut Value, patch: &Value) -> bool { | ||
| match (this, patch) { | ||
| (Value::Object(t), Value::Object(p)) => patch_map(t, p), | ||
| (Value::String(t), Value::String(p)) => *t = p.clone(), | ||
| (Value::Bool(t), Value::Bool(p)) => *t = *p, | ||
| (Value::Number(t), Value::Number(p)) => *t = p.clone(), | ||
| (Value::Array(t), Value::Array(p)) => *t = p.clone(), | ||
| (Value::Null, Value::Null) => (), | ||
| _ => return false, | ||
| }; | ||
| true | ||
| } | ||
|
|
||
| fn patch_map(this: &mut Map<String, Value>, patch: &Map<String, Value>) { | ||
| for (k, v) in patch { | ||
| match (this.get_mut(k), v) { | ||
| (Some(_), Value::Null) => { | ||
| this.remove(k); | ||
| } | ||
| (_, Value::Null) => { | ||
| // If the patch is null, then don't add it to this value. | ||
| } | ||
| (Some(t), p) => { | ||
| if !patch_value(t, p) { | ||
| println!("Warning: the patched key '{k}' has different types: {t} != {p}"); | ||
| this.insert(k.clone(), v.clone()); | ||
| } | ||
| } | ||
| (None, _) => { | ||
| this.insert(k.clone(), v.clone()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a very solid possibility that this is a pedantic comment so sorry if it is but I'd prefer if these were trait impls for something like Patch<T> with a patch(&mut self, patch: &T) -> bool method. I know that's a bit of a heavier handed solution but the this var reference and this particular pattern don't seem to be an existing pattern we follow in Nimbus code.
I may be wrong and am totally open to a larger discussion on this I just wanted to note it.
There was a problem hiding this comment.
Yes, you're absolutely right. I thought I'd take a short cut– since there was only going to one implementation of the trait, but there ended up being two.
| fn patch_experiment(experiment: &ExperimentSource, patch: &PathBuf) -> Result<Value> { | ||
| let mut value: Value = experiment.try_into()?; | ||
|
|
||
| let patch: FeatureDefaults = read_from_file(patch)?; |
There was a problem hiding this comment.
So I initially hit an issue with this in that I wrote the patch file in yaml but didn't include the yaml extension on the file.
The error just said something along the lines of "ident error line 1 column 2". It'd be nice if we could add a map_err here and say that there's an error with the patch file so the user knows where the problem lies.
Fixes EXP-3668.
This PR adds a
--patch FILEoption to all commands that accept an experiment as input:enrollfeaturesfetchinfovalidateThe patch file is expected to be structured:
{ "feature1": {}, "feature2": {} }This is the same format that
defaultsandfeatures --multiuse.The patch file can be kept by the user and used over and over, or generated on the fly, e.g. with
jq.Examples may be:
message-under-experiment, addingexperiment: "{experiment}"and changing the trigger to["ALWAYS"]:However you get the patch file:
Pull Request checklist
[ci full]to the PR title.Branch builds: add
[firefox-android: branch-name]to the PR title.