-
Notifications
You must be signed in to change notification settings - Fork 274
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
fix: do not consider implicit dependencies from action references in dead code branches #6862
Conversation
dead code branches This PR changes `getContextLookupReferences` to ignore dead code branches
Alternative approach for #6849 |
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.
This is great work and a real fun read! Together with the good test coverage this really looks good to go!
return this.getChildren() | ||
} | ||
|
||
const activeBranch = isTruthy(condition) ? this.consequent : this.alternate |
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.
This is some smart reasoning!
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.
Hahah!
}, | ||
] | ||
for (const testCase of branchTestCases) { | ||
it(`correctly avoids dead code branches (test case: ${testCase.name})`, () => { |
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.
super nice to cover all these test cases!
Agreed! Was still reading through it but |
What this PR does / why we need it:
This PR changes
getContextLookupReferences
to ignore dead codebranches
Which issue(s) this PR fixes:
Fixes an issue where we considered implicit dependencies from action references in dead code branches.
Addititional info:
There was an alternate solution attempt, here: #6849
One of the benefits of the approach of detecting dead branches is that it now also works with logical AND (
&&
), logical OR (||
), ternary expressions (? :
) and if block expressions (${if ...}
) and not just with structural if operators ($if:
yaml keys).Example / minimal repro:
Command:
garden run myrun
Expected behaviour: the deploy "mydeploy" should not be executed.
The docs for the
disabled
flag do not reflect the current behaviour where we execute disabled actions, if another action references one of their outputs. This is a separate issue that will be dealt with in a separate PR.