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

Correct expressions in GitHub Actions workflows #1952

Merged

Conversation

tunetheweb
Copy link
Contributor

@tunetheweb tunetheweb commented Sep 11, 2021

Fixes #1918

Proposed Changes

The expression syntax used by a number of workflows is incorrect.

If you use this syntax:

if: ${{ github.event_name }} == 'schedule'

this will ALWAYS evaluate to true as the evaluation happens in the ${{..}} part, and the part after the == is ignored.

You should instead evaluate the whole expression within the ${{..}} parts:

if: ${{ github.event_name == 'schedule' }}

Or alternatively the ${{..}} part is actually optional within if: statements so this works too:

if: github.event_name == 'schedule'

This means both parts of the stale job are run (the scheduled part which adds stale label to issue, and also removes it necessary), and the second part (which removes the stale label from the commented on issue only). This means the stale label is attempted to be removed twice, hence why the job fails and you get the email.

I originally noticed this for the stale job after raising #1918 but it actually is wrong in other workflows too, meaning:

  • deploys are incorrectly running (and failing) on forks.
  • permissions to release are not being checked - I was originally worried when I saw this, but don't think it is really an issue since only maintainers have access to release anyway which is when this workflow is triggered. Still it should be fixed.

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request

No change to documentation required.

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@admiralAwkbar
Copy link
Collaborator

@tunetheweb Amazing fix!
How did you track this down?

@admiralAwkbar admiralAwkbar enabled auto-merge (squash) September 14, 2021 13:31
@admiralAwkbar admiralAwkbar merged commit 1930e2f into super-linter:master Sep 14, 2021
@tunetheweb
Copy link
Contributor Author

@tunetheweb Amazing fix!
How did you track this down?

It took a lot of trial an error on my own fork to figure this out!! But it was annoying me 😁

I actually think this is very bad for GitHub Actions to allow this and silently ignore it. I raised it with the GitHub Security team before opening this PR but they didn't see it as a security issue. So opened this PR and raised this: https://github.community/t/github-actions-if-statements-can-easily-be-coded-incorrectly/200621

If you have any contacts in GitHub Actions team it would be good to point them at this!

@tunetheweb tunetheweb deleted the tunetheweb-correct-stale-job branch September 14, 2021 13:45
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.

Annoying email alert when commenting on issues
2 participants