Add test for step data update with CLI options #1939
Conversation
happz
force-pushed
the
normalize-options-properly
branch
from
f0c5216 to
6e18c3c
Compare
happz
added
bug
happz
force-pushed
the
normalize-options-properly
branch
from
6e18c3c to
00107c3
Compare
| # `self.data`, or it has been replaced with whatever came from an | ||
| # fmf node. | ||
| if value == container._default(keyname): | ||
| continue |
There was a problem hiding this comment.
This does not seem to be right: When the fmf config contains a non-default value we are not able to override it from the command line by the default one. Confirmed with this plan:
provision:
how: container
image: fedora:36
execute:
how: tmt
script: tmt --helpTrying to overide with fedora does not work:
> tmt run -av provision -h container -i fedora
/var/tmp/tmt/run-153
Found 1 plan.
/plans/example
discover
how: shell
summary: 1 test selected
/script-00
provision
how: container
image: fedora:36
name: tmt-153-IAAErkjE
full name: default-0
distro: Fedora Linux 36 (Container Image)
kernel: 6.1.18-200.fc37.x86_64
summary: 1 guest provisioned
There was a problem hiding this comment.
All of this is just wrong... It's all guessing, and it's a big smoking pile of corner cases. Let me find out what exactly breaks when this particular check is dropped, I bet there's something just waiting to bite us :)
There was a problem hiding this comment.
Summary from the call: We need to find a reliable way how to detect if a flag option was provided on the command line or not (versus set as the default). Moving to the next release because of the regression (unable to override non-default config value with default value provided on the command line).
There was a problem hiding this comment.
Right, it's the boolean value of these defaults. TL;DR, there's no way for code to tell whether -i was used on the command line, or whether the fedora string comes from the default value. For the code, these two look the same: -h container vs -h container -i fedora, both would present fedora as the value of the --image CLI option. Now, in the first case, we do not want to overwrite fmf source, in the second we do want to. How to tell the difference?
As briefly discussed, there is a couple of options, none trivial or immediately available. I'd try to look into introducing a callback that would notify us when an option was actually encountered on a command line, and we'd then simply use the value no matter what it ould be. Postponing this PR to avoid bigger harm.
| bar: List[str] = field(option='--bar', default_factory=list, multiple=True) | ||
| baz: str = 'undefined' | ||
| qux: List[str] = field(option='--qux', default_factory=list, multiple=True) | ||
| corge: bool = field(option='--corge / --no-corge', default=True, multiple=True) |
There was a problem hiding this comment.
Is multiple=True valid for a flag?
There was a problem hiding this comment.
Hmm, depends, could be. --foo --no-foo --foo might look pointless, but maybe there's a use for it. It is definitely a copy & paste issue rather than intention.
happz
force-pushed
the
normalize-options-properly
branch
from
00107c3 to
667f238
Compare
This functionality has been inside `BasePlugin`, and today a bug was reported with its handling of flag-like options. Therefore, this patch: * extracts the code into a standalone function, complementing the rest of normalization code, * fixes the bug, * fixes a few more issues discovered, mostly readability, * adds a pile of comment WRT various related issues, and * adds a unit test.
happz
force-pushed
the
normalize-options-properly
branch
from
667f238 to
68a650d
Compare
|
Any plans with this one, @happz? |
This functionality has been inside
BasePlugin, and today a bug was reported with its handling of flag-like options. Therefore, this patch: