-
Notifications
You must be signed in to change notification settings - Fork 40
enhance: evaluation result handling- include resource key address #262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
refeed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think you forgot to include terraform_plan provider related changes here :)
You mean in the PR description? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the handling of evaluation results and provider outputs by explicitly including a top-level "address" field. Key changes include:
- Updating the core evaluator result handling to copy and display the "address" in messages.
- Modifying the Terraform plan handler to conditionally add the "address" field in multiple output cases.
- Adjusting the JSON provider to separate the "address" from the metadata.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tirith/providers/terraform_plan/handler.py | Updates to conditionally add an "address" field to result dictionaries in various operation paths. |
| src/tirith/providers/json/handler.py | Refactors result creation to include a separate "address" property for JSON outputs. |
| src/tirith/core/core.py | Enhances evaluator result generation to include and display the "address" in the output message. |
refeed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Akash, thanks, could you also make sure that every result["addresses"] is always a list of strings. Example in the suggestions
| continue | ||
| for reference in expressions_val.get("references", []): | ||
| # Only get the resource type | ||
| resource_references.add(reference.split(".")[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the address from the reference
addresses.append(reference)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@refeed I am unable to understand why do we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because that's the real addresses the optype is processing. The resource var that's placed in the meta key was actually not placed correctly, it only contains one resource even though this optype is processing more than one resources
refeed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, put some comments
|
|
||
| if "addresses" in evaluator_input: | ||
| # Add addresses directly | ||
| # TODO: We need to make a model class for the `evaluator_input` and move this validation there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the type assertion mentioned in #262 (comment) here
if not isinstance(addresses, list):
raise Exception("`addresses` should be a list")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the comment to the top of line 94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# TODO: We need to make a model class for the `evaluator_input` and move this validation there
if not isinstance(addresses, list):
raise Exception("`addresses` should be a
| address = resource_change.get("address") | ||
| addresses = [address] if address is not None else [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a helper method that creates addresses from resource_change dict as I'm seeing this pattern often
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are just repeating it twice or thrice atmost. I dont think we need a helper class for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeating it twice is already a good candidate for making a helper method. It doesn't hurt much and improves readability :)
|
|
|
||
| if "addresses" in evaluator_input: | ||
| # Add addresses directly | ||
| # TODO: We need to make a model class for the `evaluator_input` and move this validation there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the comment to the top of line 94
|
|
||
| if "addresses" in evaluator_input: | ||
| # Add addresses directly | ||
| # TODO: We need to make a model class for the `evaluator_input` and move this validation there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# TODO: We need to make a model class for the `evaluator_input` and move this validation there
if not isinstance(addresses, list):
raise Exception("`addresses` should be a
| address = resource_change.get("address") | ||
| addresses = [address] if address is not None else [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeating it twice is already a good candidate for making a helper method. It doesn't hurt much and improves readability :)
|
I'll continue to work on this |
|



Pull Request Template
This pull request improves the handling of metadata and addresses in evaluation results and provider outputs. The changes ensure better compatibility, clarity, and backward support by explicitly managing the
addressfield and enhancing how metadata is processed.Enhancements to metadata and address handling:
generate_evaluator_resultinsrc/tirith/core/core.py: Updated to include themetafield only if it exists and is notNone. Additionally, theaddressfield is now explicitly handled, either directly from the input or as a fallback from themetadictionary for backward compatibility. Themessagefield is updated to include theaddresswhen available.get_valueinsrc/tirith/providers/json/handler.py: Modified to add theaddressfield as a separate property in the result dictionary instead of embedding it inmeta. This change improves clarity and aligns with the new handling approach.Description
Add Resource Address to Evaluation Results
Summary
This PR adds support for including resource addresses in evaluation results, making it easier to identify which specific resources are passing or failing evaluations.
Changes
Modified core to include resource addresses in evaluation results and messages
Updated the JSON provider to include key paths as address fields
Enhanced the pretty printer to display resource addresses in CLI output
Made address a top-level field in evaluation results for easier programmatic access
What changes are being made?
Why are these changes necessary?
Which issues or tickets does this PR close or relate to?
Type of Change
Checklist
Screenshots or Recordings (if applicable)
Additional Information