[SG-4917] feat(terraform_plan): name the resource and attribute in result messages (on main) - #287
Open
refeed wants to merge 2 commits into
Open
[SG-4917] feat(terraform_plan): name the resource and attribute in result messages (on main)#287refeed wants to merge 2 commits into
refeed wants to merge 2 commits into
Conversation
A result message only stated the comparison it made, so a check run across a
plan with 30 buckets printed 30 identical lines with no way to tell which
resource had failed. The detail was already in the result document - the
provider attaches the whole resource_change as `meta` on every output - but
nothing between the provider and the pretty printer surfaced it, so reaching it
meant piping --json into another tool.
Providers may now return an optional `context` alongside value/meta/err. The
core renders it into the front of the message and keeps it as structured fields
in the result document, so a failure reads:
[aws_s3_bucket.example (create)] acl: `"public-read"` is not equal to `"private"`
Only terraform_plan populates it, so every other provider's messages are
unchanged. Two related improvements come with it: a wildcard attribute reports
the index it resolved to (ebs_block_device.0.tags.application_acronym), which
tells apart results that used to read identically, and an "attribute is not
found" error names the resource it is about instead of repeating the same text
once per resource.
9 tasks
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
…urce type A real destroy-approval gate is one `action` evaluator with `terraform_resource_type: "*"`, so it emits a result per resource and eleven of twelve lines read `"no-op"` is not equal to `"delete"` byte for byte. The one line that matters is buried in them and names nothing, so finding the resource that is about to be destroyed meant counting to the same position in `resource_changes`. Fixtures for that shape - a plan of mostly untouched infrastructure, a few additions and one resource on its way out - plus the passing variant where nothing is being destroyed.
|
❌ The last analysis has failed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Template
Description
What changes are being made?
A result message used to state only the comparison it made.
NotEqualsprinted`"public-read"` is not equal to `"private"`and nothing else — no resource, no attribute — so a check run across a plan with 30 buckets printed 30 identical lines. For theterraform_planprovider, every message now names the resource address, its planned action and the attribute being evaluated:Providers may now return an optional
contextkey alongsidevalue/meta/err.core.generate_evaluator_resultrenders it into the front of the message via a newformat_context_prefix()inproviders/common.py, and keeps it as structured fields on the result:{ "passed": false, "message": "[aws_s3_bucket.example (create)] acl: `\"public-read\"` is not equal to `\"private\"`", "context": { "operation_type": "attribute", "resource_type": "aws_s3_bucket", "resource_address": "aws_s3_bucket.example", "action": "create", "attribute": "acl" }, "meta": { "…": "full resource_change" } }A wildcard attribute now reports the index it resolved to.
ebs_block_device.*.tags.application_acronymused to emit N results per resource that read identically; there was no way to tell which block device was the untagged one._get_exp_attributenow tracks the traversed path:attribute: '…' is not foundnow names its resource. Withterraform_resource_type: "*"this error was emitted once per resource with identical text and nometaat all.Could not find input value— the one message with no context whatsoever — now names the provider arguments that produced no value.Why are these changes necessary?
resource_changeasmetaon every output. But nothing between the provider and the pretty printer surfaced it.--jsoninto another tool." The Explorer makes the detail browsable; this puts it in the message, so a CI log is legible without opening anything. They compose rather than overlap — thecontextfield also gives the Explorer andplatform/report.pya ready-made label instead of re-deriving one frommeta.Which issues or tickets does this PR close or relate to?
Type of Change
Checklist
Additional Information
Design — why the context is composed in the core, not in the evaluators.
BaseEvaluator.evaluate(evaluator_input, evaluator_data)receives two bare values and nothing else, and all 13 evaluators have exact-dict unit tests. Threading context into each of them would mean rewriting every one of those tests for no gain, so the prefix is composed once ingenerate_evaluator_resultfrom whatevercontextthe provider chose to attach.Only
terraform_planpopulatescontexthere.json,kubernetes,infracostandsg_workflowreturn outputs without the key, so their messages are unchanged and thecontextfield is omitted from their results entirely. Verified by diffing--jsonoutput for all 7 of their fixtures against1.0.5: byte-for-byte identical.tests/core/test_output_compatibility.pyand its golden file pass untouched.Provider-argument errors stay uncontextualised (
severity_value: 99,resource_type: '…' is not found) — they are about the policy, not about a resource, so there is nothing to name.Verification
pytest: 774 passed. The 11 failures are pre-existing onmainand all need aterraformbinary onPATH(test_direct_references.py,test_e2e_inline_vars); CI has one, so those paths are covered there — on [SG-4917] feat(terraform_plan): name the resource and attribute in result messages #286 they passed on all five interpreters.tests/core/test_output_compatibility.py(golden--json) andtests/test_readme_is_current.pyboth pass unmodified.black --check src testsclean under black 25.9.0, whichmainis also clean under. Under the newest black (26.5.1) this branch adds zero complaints beyond the 13 files already flagged onmain— that job fails onmaintoday for the same reason, sincepsf/black@stablemoved forward.pydocstyleclean.tests/providers/terraform_plan/test_dot_star_attr.pyis untouched and still green — it calls_wrapper_get_exp_attributedirectly with 6 parametrized flat-list assertions, which is the regression guard for the traversal refactor. That wrapper keeps its old signature and return value; the path-tracking variant is a sibling.tests/providers/terraform_plan/test_message_context.py(10 tests, policies built inline so they don't depend on a fixture) andformat_context_prefixcases intests/providers/test_common.py.tests/providers/kubernetes/test_attribute.pyandtests/providers/json/test_get_value.pygained an assertion each pinning the "other providers unchanged" contract.