From 98208e99852852024a654a8f73dc1e63387fa671 Mon Sep 17 00:00:00 2001 From: Andrew Bernal Date: Thu, 20 Aug 2026 17:18:25 -0400 Subject: [PATCH] Do not copy unused options on --insert Click fills every discover option with a default, so --insert was writing repository and revision as unused None into the new phase. That made tmt warn about deprecated keys even when callers used url and ref, as Testing Farm does for kernelinstall. Only copy options that were really given on the command line or via environment. The filter now lives in a single place, _to_raw_step_datum(), so _patch_raw_datum() no longer needs to consult Click and only handles --update-missing semantics. Also fix the skip list to use 'allowed_how', the Click parameter name, instead of 'allowed-how' which never matched. Assisted-by: Cursor Co-authored-by: Cursor Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WooXbhjvXNBanpvtVD84Mi --- docs/releases/pending/insert-cli-defaults.fmf | 6 ++ tests/unit/test_cli.py | 46 ++++++++++++++ tmt/steps/__init__.py | 63 +++++++++---------- 3 files changed, 81 insertions(+), 34 deletions(-) create mode 100644 docs/releases/pending/insert-cli-defaults.fmf diff --git a/docs/releases/pending/insert-cli-defaults.fmf b/docs/releases/pending/insert-cli-defaults.fmf new file mode 100644 index 0000000000..44c96103b2 --- /dev/null +++ b/docs/releases/pending/insert-cli-defaults.fmf @@ -0,0 +1,6 @@ +description: | + The ``--insert`` step action no longer copies unused command-line + defaults into the new phase. Testing Farm and similar callers of + ``discover --insert --how fmf --url ...`` no longer trigger false + deprecation warnings for the obsolete ``repository`` and + ``revision`` keys. diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index fa22d49177..ef68ee1e94 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -266,3 +266,49 @@ def test_decide_colorization( monkeypatch.setattr(sys.stderr, 'isatty', lambda: testcase.simulate_tty) assert tmt.log.decide_colorization(no_color, force_color) == testcase.expected + + +def test_discover_insert_omits_unused_cli_defaults(run_tmt: 'RunTmt', tmppath: Path) -> None: + """ + ``--insert`` must not copy unused Click defaults such as ``repository`` + and ``revision`` into the new phase. + """ + + from tmt.steps.discover import Discover + + # CLI invocations are stored on the class and are not reset between + # tests, drop any leftovers before and after this test. + def _reset() -> None: + Discover.cli_invocations.clear() + Discover.cli_invocation = None + + _reset() + + root = tmppath / 'tree' + (root / '.fmf').mkdir(parents=True) + (root / '.fmf' / 'version').write_text('1\n') + (root / 'tests').mkdir() + (root / 'tests' / 'one.fmf').write_text('test: /bin/true\n') + (root / 'plans').mkdir() + (root / 'plans' / 'main.fmf').write_text('discover:\n how: fmf\nexecute:\n how: tmt\n') + + try: + result = run_tmt( + '--root', + str(root), + 'run', + '-i', + str(tmppath / 'run'), + 'discover', + '--insert', + '--how', + 'fmf', + '--test', + '/tests/one', + ) + finally: + _reset() + + assert result.exit_code == 0, result.output + assert "Field 'repository' is deprecated" not in result.output + assert "Field 'revision' is deprecated" not in result.output diff --git a/tmt/steps/__init__.py b/tmt/steps/__init__.py index bd7044c0ec..5d7fe5ba7e 100644 --- a/tmt/steps/__init__.py +++ b/tmt/steps/__init__.py @@ -1147,17 +1147,28 @@ def _apply_cli_invocations(self, raw_data: list[_RawStepData]) -> list[_RawStepD debug1(f'Update {self.__class__.__name__.lower()} phases by CLI invocations') - def _to_raw_step_datum(options: dict[str, Any]) -> _RawStepData: + def _to_raw_step_datum(invocation: 'tmt.cli.CliInvocation') -> _RawStepData: """ Convert CLI options to fmf-like raw step data dictionary. - This means dropping all keys that cannot come from an fmf node, like - keys representing CLI options. + Drop keys that cannot come from an fmf node, such as keys representing + CLI actions. Also omit options that were not really given on the + command line or via environment. Click fills every option with its + default, so a naive copy would put unused deprecated aliases such as + ``repository`` and ``revision`` into an ``--insert`` phase. """ def _iter_options() -> Iterator[tuple[str, Any]]: - for name, value in options.items(): - if name in ('update', 'update_missing', 'insert', 'allowed-how'): + for name, value in invocation.options.items(): + if name in ('update', 'update_missing', 'insert', 'allowed_how'): + continue + + value_source = invocation.option_sources.get(name) + if value_source not in ( + ParameterSource.COMMANDLINE, + ParameterSource.ENVIRONMENT, + ): + debug4(f'{name} not really given via CLI/env, omit from raw step datum') continue yield key_to_option(name), value @@ -1196,40 +1207,26 @@ def _ensure_name(raw_datum: _RawStepData) -> _RawStepData: def _patch_raw_datum( raw_datum: _RawStepData, incoming_raw_datum: _RawStepData, - invocation: 'tmt.cli.CliInvocation', missing_only: bool = False, ) -> None: """ Copy options from one phase specification onto another. Serves as a helper for "patching" a phase with options coming from - a command line. It must avoid copying options that were not really - given by user - because of how options are handled, simple - ``dict.update()`` would not do as ``incoming_raw_datum`` would - contain **all** options as long as they have a default value. - - Click is therefore consulted for each key/option, whether it was - really specified on the command line (or by an environment - variable). + a command line. ``incoming_raw_datum`` is expected to contain only + options really given by the user, see :py:func:`_to_raw_step_datum`, + therefore this helper only needs to handle ``--update-missing`` + semantics. """ debug3('raw step datum', str(raw_datum)) debug3('incoming raw step datum', str(incoming_raw_datum)) - debug3('CLI invocation', str(invocation.options)) for opt, value in incoming_raw_datum.items(): if opt == 'name': continue - key = option_to_key(opt) - value_source = invocation.option_sources.get(key) - - debug3(f'{opt=} {key=} {value=} {value_source=}') - - # Ignore CLI input if it's been provided by option's default - if value_source not in (ParameterSource.COMMANDLINE, ParameterSource.ENVIRONMENT): - debug4('value not really given via CLI/env, no effect') - continue + debug3(f'{opt=} {value=}') # Ignore CLI input if `--missing-only` has been set and datum already has the key. if missing_only and opt in raw_datum: @@ -1319,7 +1316,7 @@ def _log_raw_data(stage: str, raw_data: list[_RawStepData]) -> None: elif invocation.options.get('insert'): debug3('inserting new phase') - raw_datum = _to_raw_step_datum(invocation.options) + raw_datum = _to_raw_step_datum(invocation) raw_datum = _ensure_name(raw_datum) raw_data.append(raw_datum) @@ -1332,13 +1329,13 @@ def _log_raw_data(stage: str, raw_data: list[_RawStepData]) -> None: needle = invocation.options.get('name') if needle: - incoming_raw_datum = _to_raw_step_datum(invocation.options) + incoming_raw_datum = _to_raw_step_datum(invocation) for raw_datum in raw_data: if raw_datum['name'] != needle: continue - _patch_raw_datum(raw_datum, incoming_raw_datum, invocation) + _patch_raw_datum(raw_datum, incoming_raw_datum) break @@ -1358,15 +1355,13 @@ def _log_raw_data(stage: str, raw_data: list[_RawStepData]) -> None: needle = invocation.options.get('name') if needle: - incoming_raw_datum = _to_raw_step_datum(invocation.options) + incoming_raw_datum = _to_raw_step_datum(invocation) for raw_datum in raw_data: if raw_datum['name'] != needle: continue - _patch_raw_datum( - raw_datum, incoming_raw_datum, invocation, missing_only=True - ) + _patch_raw_datum(raw_datum, incoming_raw_datum, missing_only=True) break @@ -1392,7 +1387,7 @@ def _log_raw_data(stage: str, raw_data: list[_RawStepData]) -> None: debug2(f'postponed invocation #{i}', str(invocation.options)) pruned_raw_data: list[_RawStepData] = [] - incoming_raw_datum = _to_raw_step_datum(invocation.options) + incoming_raw_datum = _to_raw_step_datum(invocation) # In the 'tmt try image' command user can specify their # preferred image name without specifying the provision @@ -1438,10 +1433,10 @@ def _log_raw_data(stage: str, raw_data: list[_RawStepData]) -> None: ) if invocation.options.get('update_missing'): - _patch_raw_datum(raw_datum, incoming_raw_datum, invocation, missing_only=True) + _patch_raw_datum(raw_datum, incoming_raw_datum, missing_only=True) else: - _patch_raw_datum(raw_datum, incoming_raw_datum, invocation) + _patch_raw_datum(raw_datum, incoming_raw_datum) pruned_raw_data.append(raw_datum)