-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
workflows: add actionlint workflow #15210
Conversation
self-hosted-runner: | ||
# Labels of self-hosted runner in array of strings. | ||
labels: | ||
- 11-arm64 |
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.
I assume it just doesn't complain about the others?
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.
Yea, it doesn't know what to do with a dynamically generated runner label so just ignores it.
run: | | ||
brew install actionlint shellcheck | ||
|
||
# Annotations work only relative to GITHUB_WORKSPACE |
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.
Annotations should work fine (see: test-bot in Homebrew/core). Are you referring to add-matcher
or the process the matcher later does?
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.
Are you referring to
add-matcher
or the process the matcher later does?
Both. Annotations work for us otherwise because we use GitHub::Actions::Annotation
, which handles emitting the correct relative paths:
@file = self.class.path_relative_to_workspace(file) |
I couldn't get this to show annotations without moving everything into GITHUB_WORKSPACE
at Homebrew/homebrew-core#128075. I didn't bother trying here, though.
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.
Ok looks like this actions/runner#765.
.github/workflows/actionlint.yml
Outdated
defaults: | ||
run: | ||
shell: /bin/bash -e {0} |
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.
The default is bash -e {0}
. Are we avoiding another bash somewhere?
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.
IIUC correctly in containers the default is sh
. Or an sh
masquerading as bash
.
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.
Docs appear to suggest sh
is only used when bash
is not found: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell
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.
Hmm, weird. I know I've had trouble with [[
, ]]
in Homebrew/core workflows when I don't set shell: /bin/bash
.
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.
Now getting
shellcheck reported issue in this script: SC2115:warning:4:27: Use "${var:?}" to ensure this never expands to /*
After dropping this.
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.
It says it's running:
shell: /usr/bin/bash -e {0}
Is /usr/bin/bash
different? Don't think it should be with 22.04.
Though with that said, throwing that error feels like correct behaviour?
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.
Though with that said, throwing that error feels like correct behaviour?
Yes, agreed. Fixed.
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.
It's possible it might just not run shellcheck with custom shells, which could make sense.
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.
I get
/__w/_temp/b13f8fe9-518c-4fd3-af62-a31519cf8e56.sh: 5: shopt: not found
when running without setting the shell to /bin/bash
in Homebrew/core. Must have something to do with running in a container, since the shell there is reported as sh -e {0}
.
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.
Ah yes, that'll be because the shell is evaluated before the container is started, so it won't know whether bash is available.
Unless we need/want to avoid --noprofile --norc -o pipefail
, we could pass shell: bash
simply which should still allow shellcheck to run.
Some other parts of homebrew-core CI have the explicit /bin/bash
to avoid Homebrew bash which some tests may install, so those are unavoidable.
Co-authored-by: Bo Anderson <[email protected]>
This should be ready to merge. |
Thanks @carlocab! |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This will help us avoid future actionlint errors in our workflows.