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(core): don't execute disabled dependencies #5697

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Conversation

thsig
Copy link
Collaborator

@thsig thsig commented Feb 2, 2024

What this PR does / why we need it:

When executing an action that depends on a disabled dependency, the disabled dependency should be skipped (unless it's a build).

This is fixed here (in accordance with the docstring for the disabled field on action configs).

@thsig thsig requested a review from edvald February 2, 2024 10:49
When executing an action that depends on a disabled dependency, the
disabled dependency should be skipped (unless it's a build).

This is fixed here (in accordance with the docstring for the
`disabled` field on action configs).
@stefreak
Copy link
Member

stefreak commented Feb 2, 2024

Before this change, we did execute disabled actions if they were dependencies

Now the behaviour changes to silently skip executing disabled actions if other actions directly depend on them. Is that right?

I would have expected an error message or at least a warning here;

Another question that comes to my mind is if we need to treat this as a breaking change? How did 0.12 behave?

@thsig
Copy link
Collaborator Author

thsig commented Feb 2, 2024

We did execute disabled actions if they were dependencies of a non-disabled action being processed.

Now, we only execute disabled Builds in this way, but not disabled Deploys/Tests/Runs.

This is what the docs say, and that's why this user was confused about the changed behaviour from 0.12 (where we did indeed ignore disabled dependencies).

I'm not sure whether to treat this as a breaking change, since this change means we're bringing the behaviour in line with the docs for the disabled field.

... that said, I'm also cool with holding off on merging this until the next minor/non-patch release.

@thsig
Copy link
Collaborator Author

thsig commented Feb 2, 2024

Here's the relevant docstring that populates the reference docs for the disabled field:

Set this to \`true\` to disable the action. You can use this with conditional template strings to disable actions based on, for example, the current environment or other variables (e.g. \`disabled: \${environment.name == "prod"}\`). This can be handy when you only need certain actions for specific environments, e.g. only for development.
For Build actions, this means the build is not performed _unless_ it is declared as a dependency by another enabled action (in which case the Build is assumed to be necessary for the dependant action to be run or built).
For other action kinds, the action is skipped in all scenarios, and dependency declarations to it are ignored. Note however that template strings referencing outputs (i.e. runtime outputs) will fail to resolve when the action is disabled, so you need to make sure to provide alternate values for those if you're using them, using conditional expressions.

@stefreak
Copy link
Member

stefreak commented Feb 2, 2024

I think if this was what the documentation said and how 0.12 behaved like, then we should move forward with this and merge 👍 Thanks for the clarification @thsig

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.

Good work, LGTM 👍

@vvagaytsev vvagaytsev added this pull request to the merge queue Feb 2, 2024
Merged via the queue into main with commit 5bcb096 Feb 2, 2024
43 checks passed
@vvagaytsev vvagaytsev deleted the skip-disabled-deps branch February 2, 2024 12:43
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.

None yet

3 participants