diff --git a/CHANGELOG.md b/CHANGELOG.md index a34f4474..79662ed0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 once per resource with identical text. - `core`: "Could not find input value" now names the provider arguments that produced no value. +### Fixed +- **Verdict change.** A resource skipped through `error_tolerance` no longer overwrites the + verdict of the resources evaluated before it. An evaluator now fails if any resource fails, + passes if none fail and at least one was evaluated, and is skipped only when every resource + was tolerated away. Previously a skip reset the verdict to skipped, so a violating resource + followed by a destroyed one (severity 0, tolerated at every `error_tolerance`) disappeared + from `eval_expression` and the policy reported no verdict; under `!id` it passed. The result + depended on the order of `resource_changes`. Plans that mix compliant and destroyed + resources now pass instead of exiting 1, and plans that mix violating and destroyed + resources now exit 3 instead of 1. (#293) + ### Notes - The local evaluation surface is untouched. `ui` is dispatched before the flat parser, like `platform`, so `--json` output remains byte-identical to the golden file. diff --git a/src/tirith/core/core.py b/src/tirith/core/core.py index 81e25147..ffdd8054 100644 --- a/src/tirith/core/core.py +++ b/src/tirith/core/core.py @@ -97,7 +97,6 @@ def generate_evaluator_result(evaluator_obj, input_data, provider_module): evaluator_instance = evaluator_class() evaluation_results = [] - has_evaluation_passed = True # If there are no evaluator inputs, it means the provider didn't find any resources # In this case, the evaluation should fail @@ -105,7 +104,11 @@ def generate_evaluator_result(evaluator_obj, input_data, provider_module): has_evaluation_passed = False evaluation_results = [{"passed": False, "message": _no_input_value_message(provider_inputs)}] else: - # Track if we've had at least one valid evaluation (not skipped) + # Roll-up across resources: any failure fails the evaluator; otherwise it passes if at least + # one resource was actually evaluated; only when every resource was tolerated away is the + # verdict None (skipped). Tracked as two flags and decided once at the end, so a skip can + # never overwrite a failure or a pass that came before it (issue #293). + has_failure = False has_valid_evaluation = False for evaluator_input in evaluator_inputs: @@ -117,7 +120,7 @@ def generate_evaluator_result(evaluator_obj, input_data, provider_module): # reads as a genuine violation. if evaluator_input.get("err") and not isinstance(evaluator_input["value"], ProviderError): evaluation_results.append({"passed": False, "message": evaluator_input["err"]}) - has_evaluation_passed = False + has_failure = True continue if isinstance(evaluator_input["value"], ProviderError) and evaluator_input.get("err", None): @@ -130,12 +133,11 @@ def generate_evaluator_result(evaluator_obj, input_data, provider_module): if severity_value > evaluator_error_tolerance: err_result.update(dict(passed=False)) evaluation_results.append(err_result) - has_evaluation_passed = False + has_failure = True continue - # Mark as skipped evaluation + # Within tolerance: this resource is skipped and does not touch the verdict err_result.update(dict(passed=None)) evaluation_results.append(err_result) - has_evaluation_passed = None continue evaluation_result = evaluator_instance.evaluate(evaluator_input["value"], evaluator_data) @@ -150,10 +152,13 @@ def generate_evaluator_result(evaluator_obj, input_data, provider_module): has_valid_evaluation = True if not evaluation_result["passed"]: - has_evaluation_passed = False + has_failure = True - # If all evaluations were skipped, we need to make sure the overall result is 'None' - if not has_valid_evaluation and has_evaluation_passed is None: + if has_failure: + has_evaluation_passed = False + elif has_valid_evaluation: + has_evaluation_passed = True + else: has_evaluation_passed = None result["result"] = evaluation_results diff --git a/tests/core/test_core.py b/tests/core/test_core.py index 858e8b06..a409cf36 100644 --- a/tests/core/test_core.py +++ b/tests/core/test_core.py @@ -215,3 +215,75 @@ def test_generate_evaluator_result_bare_provider_err_ignores_error_tolerance(): assert result["passed"] is False, "a malformed provider call must not be skipped" assert result["result"][0]["passed"] is False + + +def _skip(msg="tolerated"): + return {"value": ProviderError(severity_value=0), "err": msg} + + +def _rollup(inputs, error_tolerance=0): + """Run generate_evaluator_result over `inputs` with MockEvaluator and return the verdict.""" + evaluator_obj = { + "id": "test_evaluator", + "provider_args": {"operation_type": "attribute", "key": "value"}, + "condition": {"type": "Equals", "value": "expected_value", "error_tolerance": error_tolerance}, + } + with patch("tirith.core.core.get_evaluator_inputs_from_provider_inputs", return_value=inputs): + with patch("tirith.core.core.EVALUATORS_DICT", {"Equals": MockEvaluator}): + return generate_evaluator_result(evaluator_obj, {}, "test_provider") + + +@mark.passing +@pytest.mark.parametrize( + "inputs", + [ + [{"value": "resource2"}, _skip()], # violation first, then a tolerated skip (issue #293) + [_skip(), {"value": "resource2"}], # the same two resources, swapped + ], + ids=["fail-then-skip", "skip-then-fail"], +) +def test_generate_evaluator_result_skip_never_erases_a_failure(inputs): + """A tolerated skip must not turn a failing evaluator into a skipped one, whatever the order. + + Regression for #293: a destroyed resource (severity 0, tolerated at every error_tolerance) + after a violating resource used to reset the verdict to None, and the None id was then removed + from eval_expression, so the violation vanished. + """ + result = _rollup(inputs) + assert result["passed"] is False + assert sorted((r["passed"] for r in result["result"]), key=str) == [False, None] + + +@mark.passing +@pytest.mark.parametrize( + "inputs", + [ + [{"value": "resource1"}, _skip()], + [_skip(), {"value": "resource1"}], + [{"value": "resource1"}, _skip(), {"value": "resource1"}], + ], + ids=["pass-then-skip", "skip-then-pass", "pass-skip-pass"], +) +def test_generate_evaluator_result_skip_never_erases_a_pass(inputs): + """A skipped resource alongside passing ones leaves a passing verdict, not a skipped one. + + Before #293 was fixed, [PASS, skip] reported "Passed: 0 Failed: 0 Skipped: 1" and exited 1 for a + plan that had a compliant resource next to a destroyed one. + """ + result = _rollup(inputs) + assert result["passed"] is True + + +@mark.passing +def test_generate_evaluator_result_all_skipped_is_none(): + """Only when every resource was tolerated away is the evaluator skipped as a whole.""" + result = _rollup([_skip(), _skip()]) + assert result["passed"] is None + assert all(r["passed"] is None for r in result["result"]) + + +@mark.passing +def test_generate_evaluator_result_skip_never_erases_a_bare_provider_error(): + """A malformed provider call fails hard even when a tolerated skip follows it.""" + result = _rollup([{"value": None, "err": "attribute_to_get is not supported"}, _skip()]) + assert result["passed"] is False