Skip to content
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

Ensure we have all references for output preconditions #32464

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jan 5, 2023

Output references from error_message were not being tracked. Also fix the early return in referencesForOutput, which could skip preconditions altogether. The small slice allocation optimization is not really needed here, since this is not a hot path at all.

Fixes #32447

Output references must also include the error_message expression.
Fix the early return in referencesForOutput, which could skip
preconditions. The small slice allocation optimization is not really
needed here, since this is not a hot path at all.
These references were getting dropped, which could cause the referenced
nodes to be pruned from the graph.
@jbardin jbardin added the 1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Jan 5, 2023
@jbardin jbardin requested a review from a team January 5, 2023 15:05
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I can see how this happened... the initial preconditions/postconditions prototype required error messages to be just static strings so we were evaluating them in the config decoder rather than at runtime. We later moved the error message evaluation into the same spot as the condition evaluation, but apparently neglected to tell the References function about that.

This seems like a fine fix for that oversight! 👍

@jbardin jbardin merged commit e200f53 into main Jan 5, 2023
@jbardin jbardin deleted the jbardin/output-check-refs branch January 5, 2023 19:29
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The final value (variable) is missing in precondition error_message.
2 participants