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

fix(template): avoid premature disabled flag evaluation on actions #6448

Merged
merged 6 commits into from
Sep 17, 2024

Conversation

vvagaytsev
Copy link
Collaborator

@vvagaytsev vvagaytsev commented Sep 17, 2024

What this PR does / why we need it:

  • Do early disabled flag evaluation only for actions with duplicate names.
  • Deny the usage of var and variables contexts in early disabled flag evaluation.

The contexts var and variables contexts are not fully resolved at that stage.
That can lead to incorrectly resolved disabled flag values.

Which issue(s) this PR fixes:

Patches #4805, #5686, #6293, and #6406.

Special notes for your reviewer:

@vvagaytsev vvagaytsev force-pushed the fix/template-strings-in-disabled-flag-new branch from 3d90797 to f279492 Compare September 17, 2024 14:49
@vvagaytsev vvagaytsev changed the title chore: do not resolve disable flag on config scan fix(template): avoid premature disabled flag evaluation on actions Sep 17, 2024
@vvagaytsev vvagaytsev requested a review from stefreak September 17, 2024 14:50
@vvagaytsev vvagaytsev marked this pull request as ready for review September 17, 2024 14:50
@vvagaytsev vvagaytsev force-pushed the fix/template-strings-in-disabled-flag-new branch from f279492 to 9a1023e Compare September 17, 2024 14:56
@vvagaytsev vvagaytsev marked this pull request as draft September 17, 2024 14:58
@vvagaytsev vvagaytsev marked this pull request as ready for review September 17, 2024 15:00
@vvagaytsev vvagaytsev force-pushed the fix/template-strings-in-disabled-flag-new branch from b3c47dc to fb6a6a1 Compare September 17, 2024 15:31
stefreak
stefreak previously approved these changes Sep 17, 2024
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Nice, this is probably a workable compromise for now.

I'm still hoping we can either lift this limitation by refactoring how the action resolution works, or introduce a more consistent behaviour in a breaking change e.g. in 0.14

@vvagaytsev vvagaytsev force-pushed the fix/template-strings-in-disabled-flag-new branch from fb6a6a1 to db08407 Compare September 17, 2024 16:03
@vvagaytsev vvagaytsev requested a review from stefreak September 17, 2024 16:05
stefreak
stefreak previously approved these changes Sep 17, 2024
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Nice! 👏

core/src/garden.ts Outdated Show resolved Hide resolved
vvagaytsev and others added 5 commits September 17, 2024 18:31
* Do early `disabled` flag evaluation only for actions with duplicate names.
* Deny the usage of `var` and `variables` contexts in early `disabled` flag evaluation.

The contexts `var` and `variables` contexts are not fully resolved at that stage.
That can lead to incorrectly resolved `disabled` flag values.

Co-authored-by: Steffen Neubauer <[email protected]>
Co-authored-by: Steffen Neubauer <[email protected]>
@vvagaytsev vvagaytsev force-pushed the fix/template-strings-in-disabled-flag-new branch from 9001fcf to 154c967 Compare September 17, 2024 16:31
@vvagaytsev vvagaytsev requested a review from stefreak September 17, 2024 16:31
stefreak
stefreak previously approved these changes Sep 17, 2024
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Awesome 👍

name: run-script
type: exec
var:
$merge: ${actions.run["run-script"].var}
Copy link
Member

Choose a reason for hiding this comment

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

What happens if I use actions context directly in the disabled flag? Should we deny that as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's already denied, the error will be thrown:

Invalid template string (...) at path disabled: Could not find key actions. Available keys: command, datetime, environment, git, inputs, local, parent, project, secrets, template, this, var and variables.

Co-authored-by: Steffen Neubauer <[email protected]>
@vvagaytsev vvagaytsev added this pull request to the merge queue Sep 17, 2024
Merged via the queue into main with commit c0e9065 Sep 17, 2024
40 checks passed
@vvagaytsev vvagaytsev deleted the fix/template-strings-in-disabled-flag-new branch September 17, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants