Add possibility to override script values #138
Conversation
docs/README.md
Outdated
|
|
||
| `template` command flags: | ||
|
|
||
| * `-c`, `--config string`: (optional) Create the specified scenario setup file. Defaults to `template.json`. | ||
| * `-f`, `--force`: Overwrite existing scenario setup file. | ||
| * `-h`, `--help`: Show the help for the `template` command. | ||
|
|
||
| #### Using script overrides | ||
|
|
||
| Script overrides overrides a value pointed to by a path to its key. If the key doesn't exist in the script there will be a not found error, even if it's a valid value according to config. |
There was a problem hiding this comment.
A script override overrides a value..
or
Script overrides override values..
There was a problem hiding this comment.
not-found-error? "not found"-error?
There was a problem hiding this comment.
If the key doesn't exist in the script there will an error, even if it's a valid value according to config.
?
| return cfgJSON, overrides, errors.Errorf("malformed override: %s, should be in the form key.path=value", kvp) | ||
| } | ||
| path := helpers.DataPath(kvSplit[0]) | ||
| rawOrig, err := path.Lookup(cfgJSON) |
There was a problem hiding this comment.
Still not entirely sure about this one 🤔 Also if the override is a json object, the individual keys won't be checked. I do understand it's a good thing to throw errors whenever possible, though, so removing it outright is not the best option either.
There was a problem hiding this comment.
Not an easy one. My thinking, right or not, is that mistakes will more likely be done when writing the path, if an entire object is used it's at least more likely to be copy-pasted. My vote is on keeping the check, but agree not optimal, don't have a better solution.
There was a problem hiding this comment.
Yeah I tend to agree if the choice is between keeping it and removing it outright.
|
Should I add an option to read from file? Was thinking the format then read line by line and execute as overrides. |
|
That sounds like a good idea. I can foresee a situation where there are too many overrides in the scheduler to be able to pass via the command.. |
|
Either that or the ability to pass overrides over stdin |
Hmm true, will keep that in mind too |
It would also make sense for the file override to be ordinary json. |
|
There is actually a simple way of checking non-existing fields when unmarshalling json. This is not 100% related to this PR, but if we add this when unmarshalling the config in general we get some more safety when doing overrides. |
|
would not play nice with at least my way of work. I usually "comment" settings in actions by prefixing key with "_" or similar when I want temporarly want to use something else. Not sure how common that is though |
|
Also doesn't play well with the UnmarshalJSON interface which we use a lot of |
Yes, this is a con. There are comments in yaml 🙊
Yes, looks like the the creation of decoder has to be scattered all over the place. There must be a way not to 🤔 |
|
will keep current implementation of checking the path as a form of error handling, since other kinds seems to get messy. This can be re-visited at a later point when we see some adoption and usage and better can determine issues. |
Checklist
Squash commit message
Add possibility to override script values
Info
https://github.com/qlik-oss/gopherciser/blob/enhancement/536-ScriptOverrides/docs/README.md#using-script-overrides