Bosh simulate consistency patch #605
Conversation
…dified LocalExecutor.generateRandomParams. To be discussed (can no longer loop _randomFillInDict)
|
@gkiar would you like to review and test this one? |
|
@glatard @DarrinFong yep - sorry for the delay, I'll get to this tomorrow :) |
|
@DarrinFong the changes look good, thanks! However, I'm still getting the exact same error in the config file case as is documented in #578 . Could you add the descriptor I pasted in that issue to the test suite, and verify consistent performance? Thanks for the hard work! |
|
Hmm @DarrinFong, could it perhaps be the way the command-line utility is invoking this? I've just checked out your last commit and did/got the following: [DarrinFong/bsimulate_inconsistency_#578] gkiar $ git log -1
commit d986441e55d665453831e25424f2d43401620e6b (HEAD -> DarrinFong/bsimulate_inconsistency_#578)
Author: darrinfong <darrinfong@gmail.com>
Date: Tue Apr 28 16:45:59 2020 -0400
python string incompatibility fix
[DarrinFong/bsimulate_inconsistency_#578] gkiar $ cat tmp.json
{
"name": "tool",
"tool-version": "1",
"description": "a tool",
"command-line": "tool_cli [CONFIG_FILE]",
"schema-version": "0.5",
"inputs": [
{
"id": "value",
"name": "value",
"type": "String",
"value-key": "[VALUE]",
"value-choices": ["yes", "no"],
"default-value": "no",
"optional": true
}
],
"output-files": [
{
"id": "config_file",
"name": "Configuration file",
"value-key": "[CONFIG_FILE]",
"path-template": "config.toml",
"file-template": [
"hi__",
"value = [VALUE]",
"__bye"
]
}
]
}
[DarrinFong/bsimulate_inconsistency_#578] gkiar $ bosh exec simulate tmp.json && cat config.toml
Generated Command:
tool_cli config.toml
hi__
value = no
__bye
[DarrinFong/bsimulate_inconsistency_#578] gkiar $ bosh example tmp.json && cat config.toml
{}
hi__
__bye |
|
@gkiar If I remember correctly |
|
Hey @DarrinFong I think you're right in the case of common descriptors/invocations, but I think in a config file the default values should always be populated as having empty lines in a config file where some input is expected may cause issue. In this case, I think it's safer to assume the value isn't a default in the sense of "will be assumed" but rather as "the expected value" of a parameter. What do you think? Could you maybe build in a switch that automatically forces In any case, I think we should have consistency between our simulation methods -- I wouldn't expect Thanks for the hard work on this! |
|
Hey @gkiar, I agree we should expect consistent behaviour, so I went back and thought about this: The purpose of From that, we shouldn't expect Let me know if I made an incorrect assumption on |
|
@DarrinFong -- I agree completely, great point and yes let's make that happen! Thanks for the great work on this, it's hugely appreciated! |
…is generated from example
|
Looks great :) Nicely done, @DarrinFong ! |
Related issues
#578
Purpose
bosh exec simulateshould yield consistent results regardless of an invocation's presence through option-i.Current behaviour
Descriptor's
optional: trueinputs withdefault-valueare not added to simulated command-line output, but are added when an invocation (created by bosh example) is supplied.New behaviour
optional: trueinputs withdefault-valueare added to simulated command-line output consistently.Implementation Detail
In
simulate,localExec.addDefaultValuesis called beforeLocalExecutor.generateRandomParams.LocalExecutor.generateRandomParamsno longer takes argumentnfor the number of iterations to attempt. This is to prevent theLocalExecutor.in_dictfrom clearing before each iteration (in_dictcontains inputs to use when generating random param values).