diff --git a/CHANGELOG.md b/CHANGELOG.md index f27a69f2..57a8fe4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -256,6 +256,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 engines, and refuses with `ERR_APPLY_BUILD_IDENTIFIER_INVALID`, which names the id, says nothing was applied and suggests a rename. A mode without a build phase, and a contract without builds, are not checked, as before. +- **A mistyped `--build-id` provisioned the product, then found no build.** The build runner keeps + the builds whose id equals `--build-id` and, with none left, logged `Build not found` and exited + 1, but on the OpenTofu engine it starts after `tofu apply`, so the infrastructure was created + first. A build mode now checks `--build-id` against the contract's builds before the apply + hooks, `tofu` and the runner, on both engines, and refuses with `ERR_APPLY_BUILD_ID_NOT_FOUND`, + which names the id, lists the ids the contract has and says nothing was applied. Ids match + exactly, as in the runner. An empty `--build-id` and a contract without builds are not checked, + as before. The Command Center run now reports `apply_build_id_not_found`, where it reported + `build_not_found:`. - **`fluid diff --baseline` reported nothing for column changes in a 0.7.x contract.** It read columns from `exposes[].schema`, but a 0.7.x contract keeps them in `exposes[].contract.schema` diff --git a/fluid_build/_error_catalog.py b/fluid_build/_error_catalog.py index 8918e49d..405eb3ba 100644 --- a/fluid_build/_error_catalog.py +++ b/fluid_build/_error_catalog.py @@ -276,6 +276,14 @@ def slug_for(event: str) -> str: ], None, ), + "apply_build_id_not_found": ( + [ + "Pass one of the available build ids as --build-id: ids match exactly, case included", + "Run 'fluid apply --mode amend-and-build' without --build-id to run " + + "every build", + ], + None, + ), "output_write_failed": ( ["Check that the --out path is writable and the parent directory exists"], None, diff --git a/fluid_build/build_runners/base.py b/fluid_build/build_runners/base.py index 3fd4e3af..78da7ebb 100644 --- a/fluid_build/build_runners/base.py +++ b/fluid_build/build_runners/base.py @@ -1411,7 +1411,11 @@ def run_builds_from_args( report = current_apply_run() product_id = str(contract.get("id") or "") - # Filter builds if specific ID requested + # Filter builds if specific ID requested. ``fluid apply`` refuses an id no + # build has before it applies anything + # (``cli/apply.py::_gate_selected_build_for_apply``), so on the OpenTofu + # engine it is not refused here after ``tofu apply``; this one stays as the + # backstop for any caller that skips it. if args.build_id: builds = [b for b in builds if b.get("id") == args.build_id] if not builds: diff --git a/fluid_build/cli/apply.py b/fluid_build/cli/apply.py index 40b80600..c702ea26 100644 --- a/fluid_build/cli/apply.py +++ b/fluid_build/cli/apply.py @@ -346,6 +346,47 @@ def _gate_build_ids_for_apply(contract: Dict[str, Any]) -> None: ) from exc +def _gate_selected_build_for_apply(contract: Dict[str, Any], build_id: Optional[str]) -> None: + """Refuse, before anything is applied, a ``--build-id`` that names no build. + + The build runner keeps the builds whose id equals ``--build-id`` and, with + none left, logs "Build not found" and returns 1. On the OpenTofu engine it + runs after ``tofu apply``, so a mistyped id provisioned the product and + then failed. This is the runner's own match + (``build_runners.base.run_builds_from_args``), run ahead of the hooks, + ``tofu`` and the runner on both engines. Like the runner, it checks nothing + for an empty ``--build-id``, which filters nothing, or for a contract with + no builds, where the runner returns before it filters. + + Raises: + CLIError: ``apply_build_id_not_found``. + """ + builds = contract.get("builds") or [] + if not (isinstance(build_id, str) and build_id) or not builds: + return + # The runner's filter: an exact match, case and spaces included. + if any(isinstance(build, dict) and build.get("id") == build_id for build in builds): + return + available = [build["id"] for build in builds if isinstance(build, dict) and build.get("id")] + if available: + why, fix = "no build of the contract has that id", "Pass one of the available ids" + else: + why, fix = "none of the contract's builds declares an id", "Give the build you mean an id" + raise CLIError( + 1, + "apply_build_id_not_found", + { + "build_id": repr(build_id), + "available": available, + "hint": ( + "--build-id selects the build whose id equals it exactly, case and spaces " + f"included, and {why}. {fix}, or drop --build-id to run every build. " + "Nothing was applied." + ), + }, + ) + + def _log_apply_failure(logger: logging.Logger, start_time: float, error: Exception) -> None: """Record a pre-dispatch ``apply_contract`` failure for the structured log.""" log_operation_failure(logger, "apply_contract", error=error, duration=time.time() - start_time) @@ -1962,12 +2003,14 @@ def run(args, logger: logging.Logger) -> int: # skip it, so a schema-invalid contract reached ``tofu`` or the build # runner and could exit 0. One call per run, on the one loaded copy. # A build mode also checks the ids the build phase turns into paths, which - # the schema admits more of: on the OpenTofu engine the runner would only - # refuse one after ``tofu apply``. + # the schema admits more of, and then that ``--build-id`` names a build, in + # the runner's order: on the OpenTofu engine the runner would only refuse + # either after ``tofu apply``. try: _gate_contract_for_apply(source_contract, logger) if needs_build(resolved_mode): _gate_build_ids_for_apply(source_contract) + _gate_selected_build_for_apply(source_contract, resolved_build_id) except CLIError as exc: _log_apply_failure(logger, start_time, exc) raise diff --git a/tests/cli/test_apply_build_ids_before_tofu.py b/tests/cli/test_apply_build_ids_before_tofu.py index a68f329d..af4086f0 100644 --- a/tests/cli/test_apply_build_ids_before_tofu.py +++ b/tests/cli/test_apply_build_ids_before_tofu.py @@ -22,6 +22,10 @@ ``--mode amend-and-build`` provisioned the product and then failed with "Unexpected error", because ``IdentifierViolation`` had no typed mapping. +A ``--build-id`` that names no build failed late the same way: the runner keeps the +builds whose id equals it and, with none left, logs ``Build not found`` and returns 1, +after ``tofu apply`` on the OpenTofu engine. + These tests drive ``run()`` through the real argparse surface. ``tofu`` and the apply hooks are recorders; the build runner is the real one, behind a spy, so "refused first" is asserted as "``tofu``, the hooks and the runner never ran". @@ -33,7 +37,7 @@ import json import logging from pathlib import Path -from typing import Any, Dict, Iterator +from typing import Any, Dict, Iterator, Optional from unittest import mock import pytest @@ -355,3 +359,172 @@ def test_the_runner_still_refuses_on_its_own(self, tmp_path: Path) -> None: ) def test_what_it_does_not_check(self, contract: Dict[str, Any]) -> None: _ids.validate_build_ids(contract) + + +# ``--build-id`` values that name no build of ``_CONTRACT``. The runner compares ids +# exactly, so a near miss is as unknown as a typo. +_UNKNOWN_BUILD_IDS = [ + pytest.param("make_row", id="typo"), + pytest.param("Make_Rows", id="case"), + pytest.param("make_rows ", id="trailing-space"), + pytest.param("make", id="prefix"), +] + + +def _with_build_ids(*build_ids: Optional[str]) -> Dict[str, Any]: + """``_CONTRACT`` with one build per id; ``None`` is a build that declares no id.""" + contract = copy.deepcopy(_CONTRACT) + template = contract["builds"][0] + contract["builds"] = [] + for build_id in build_ids: + build = copy.deepcopy(template) + if build_id is None: + del build["id"] + else: + build["id"] = build_id + contract["builds"].append(build) + return contract + + +@pytest.mark.usefixtures("workspace") +class TestAnUnknownBuildIdIsRefusedFirst: + """``--build-id`` names no build of the contract. + + The runner keeps the builds whose id equals ``--build-id`` and, with none left, + logs ``Build not found`` and returns 1. On the OpenTofu engine that came after + ``tofu apply``: a mistyped id provisioned the product, then the run failed. + """ + + @pytest.mark.parametrize("form", ["yaml", "plan"]) + @pytest.mark.parametrize("build_id", _UNKNOWN_BUILD_IDS) + @pytest.mark.parametrize("mode,extra", _BUILD_MODES) + def test_refused_before_tofu( + self, + tofu: mock.Mock, + hooks: mock.Mock, + runner: mock.Mock, + form: str, + build_id: str, + mode: str, + extra: Any, + ) -> None: + src = _source(form, _CONTRACT, mode=mode) + with pytest.raises(CLIError) as exc: + _apply(src, "--mode", mode, *extra, "--build-id", build_id) + tofu.assert_not_called() + hooks.assert_not_called() + runner.assert_not_called() + assert exc.value.event == "apply_build_id_not_found" + assert exc.value.context["build_id"] == repr(build_id) + assert exc.value.context["available"] == ["make_rows"] + + @pytest.mark.usefixtures("native") + def test_native_engine_refuses_with_the_same_typed_error(self, runner: mock.Mock) -> None: + """The native engine applies nothing before its builds, but its refusal was a + log line and exit 1.""" + src = _source("yaml", _CONTRACT, mode="amend-and-build") + with pytest.raises(CLIError) as exc: + _apply(src, "--mode", "amend-and-build", "--build-id", "make_row") + runner.assert_not_called() + assert exc.value.event == "apply_build_id_not_found" + + def test_it_lists_the_build_ids_in_contract_order(self, tofu: mock.Mock) -> None: + src = _source("yaml", _with_build_ids("make_rows", "load_orders"), mode="amend-and-build") + with pytest.raises(CLIError) as exc: + _apply(src, "--mode", "amend-and-build", "--build-id", "load_order") + assert exc.value.context["available"] == ["make_rows", "load_orders"] + + def test_builds_that_declare_no_id_match_no_build_id( + self, tofu: mock.Mock, runner: mock.Mock + ) -> None: + """``builds[].id`` is optional, and the runner matches no ``--build-id`` to a + build without one.""" + src = _source("yaml", _with_build_ids(None), mode="amend-and-build") + with pytest.raises(CLIError) as exc: + _apply(src, "--mode", "amend-and-build", "--build-id", "make_rows") + tofu.assert_not_called() + runner.assert_not_called() + assert exc.value.context["available"] == [] + assert "none of the contract's builds declares an id" in exc.value.context["hint"] + + def test_an_unusable_id_is_refused_first(self, tofu: mock.Mock) -> None: + """The runner checks the ids before it filters, and apply keeps that order.""" + src = _source("yaml", _with("contract.id", "1orders"), mode="amend-and-build") + with pytest.raises(CLIError) as exc: + _apply(src, "--mode", "amend-and-build", "--build-id", "make_row") + assert exc.value.event == "apply_build_identifier_invalid" + + def test_the_refusal_says_what_to_change(self, tofu: mock.Mock) -> None: + src = _source("yaml", _CONTRACT, mode="amend-and-build") + with pytest.raises(CLIError) as exc: + _apply(src, "--mode", "amend-and-build", "--build-id", "make_row") + hint = exc.value.context["hint"] + assert "Nothing was applied" in hint + assert "drop --build-id" in hint + assert exc.value.suggestions, "the catalog gives the event its suggestions" + + def test_the_cli_renders_it_as_a_typed_error( + self, tofu: mock.Mock, capsys: pytest.CaptureFixture[str] + ) -> None: + """Through ``main()``: the slug, the id asked for and the ids there are.""" + src = _source("yaml", _CONTRACT, mode="amend-and-build") + argv = ["apply", src, "--mode", "amend-and-build", "--build-id", "make_row", "--yes"] + assert cli_mod.main(argv) == 1 + out = capsys.readouterr().out + assert "ERR_APPLY_BUILD_ID_NOT_FOUND" in out + assert "'make_row'" in out + assert "['make_rows']" in out + tofu.assert_not_called() + + @pytest.mark.parametrize("build_id", _UNKNOWN_BUILD_IDS) + def test_the_runner_still_refuses_on_its_own( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture, build_id: str + ) -> None: + """Kept as the backstop for a caller that skips ``fluid apply``. It refuses the + same near misses: both compare ids exactly.""" + import argparse + + path = tmp_path / "contract.fluid.yaml" + path.write_text("{}", encoding="utf-8") + args = argparse.Namespace(contract=str(path), env=None, build_id=build_id) + with caplog.at_level(logging.ERROR, logger=LOG.name): + rc = runner_base.run_builds_from_args(args, LOG, contract=copy.deepcopy(_CONTRACT)) + assert rc == 1 + assert f"Build not found: {build_id}" in caplog.text + + +@pytest.mark.usefixtures("workspace") +class TestAKnownBuildIdStillRuns: + """The check refuses exactly the ``--build-id`` values the runner would refuse.""" + + @pytest.mark.parametrize("form", ["yaml", "plan"]) + @pytest.mark.parametrize("build_id", ["make_rows", "load_orders"]) + def test_it_reaches_tofu_then_the_runner( + self, tofu: mock.Mock, form: str, build_id: str + ) -> None: + src = _source(form, _with_build_ids("make_rows", "load_orders"), mode="amend-and-build") + with mock.patch("fluid_build.build_runners.run_builds_from_args", return_value=0) as run: + assert _apply(src, "--mode", "amend-and-build", "--build-id", build_id) == 0 + tofu.assert_called_once() + run.assert_called_once() + assert run.call_args.args[0].build_id == build_id + + def test_an_empty_build_id_filters_nothing(self, tofu: mock.Mock) -> None: + """The runner runs every build for ``--build-id ''``, so it is not refused.""" + src = _source("yaml", _CONTRACT, mode="amend-and-build") + with mock.patch("fluid_build.build_runners.run_builds_from_args", return_value=0) as run: + assert _apply(src, "--mode", "amend-and-build", "--build-id", "") == 0 + tofu.assert_called_once() + run.assert_called_once() + + def test_a_contract_without_builds_is_not_checked( + self, tofu: mock.Mock, runner: mock.Mock + ) -> None: + """With no builds the runner returns 0 before it filters, so a ``--build-id`` is + not refused there either.""" + contract = copy.deepcopy(_CONTRACT) + del contract["builds"] + src = _source("yaml", contract, mode="amend-and-build") + assert _apply(src, "--mode", "amend-and-build", "--build-id", "make_rows") == 0 + tofu.assert_called_once() + runner.assert_called_once() diff --git a/tests/cli/test_apply_reports_to_command_center.py b/tests/cli/test_apply_reports_to_command_center.py index c60e76a4..80782468 100644 --- a/tests/cli/test_apply_reports_to_command_center.py +++ b/tests/cli/test_apply_reports_to_command_center.py @@ -592,15 +592,21 @@ def test_a_failed_build_is_reported_with_its_build_id(product, tofu, build, cc, def test_a_build_id_that_is_not_in_the_contract_is_the_reason( product, tofu, build, cc, monkeypatch ): + """Refused before ``tofu`` runs: the run reports the refusal and no infrastructure.""" url, recorder = cc _configure(monkeypatch, url) _with_build(product) - assert _apply(product, "--mode", "amend-and-build", "--build-id", "no_such_build") == 1 + with pytest.raises(CLIError) as exc: + _apply(product, "--mode", "amend-and-build", "--build-id", "no_such_build") + assert exc.value.event == "apply_build_id_not_found" + (post,) = _by(recorder, "POST") (patch,) = _by(recorder, "PATCH") - assert patch["body"]["result"]["error_event"] == "build_not_found:no_such_build" - assert patch["body"]["current_phase"] == "build" + assert post["body"]["metadata"]["product_id"] == "bronze.customer_subscriptions" + result = patch["body"]["result"] + assert result["error_event"] == "apply_build_id_not_found" + assert "planned_changes" not in result assert build["calls"] == 0