-
Notifications
You must be signed in to change notification settings - Fork 160
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
add shellcheck to CI #1423
add shellcheck to CI #1423
Conversation
b2f5cba
to
dc05677
Compare
.github/workflows/shellcheck.yml
Outdated
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v2 | ||
- name: Run shellcheck |
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.
What about adding these two lines to the lint job to reduce complexity in the workflows?
https://github.com/ChainSafe/forest/blob/main/.github/workflows/rust.yml#L100
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 was considering this, then I looked at the workflow name and I abandoned this idea - all the jobs inside seem to be strictly related to Rust. On the other hand, if we'd rename it to e.g. Forest CI
or some better name, then it would fit nicely the lint job. What do you think?
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 agree, however, renaming it means we need to mess with all the PRs again. Maybe just keep it in shell.yml
for now.
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.
Sounds good to me!
@@ -76,4 +76,4 @@ for endpoint in ${RPC_ENDPOINTS[@]}; do | |||
done | |||
|
|||
# Kill forest daemon | |||
ps -ef | grep forest | grep -v grep | awk '{print $2}' | xargs kill | |||
pgrep forest | xargs kill |
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.
👍🏼 😆
.github/workflows/shellcheck.yml
Outdated
@@ -0,0 +1,18 @@ | |||
name: ShellCheck |
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.
name: ShellCheck | |
name: Shell |
Let's name it also Shell
so in case we need to add more later.
Summary of changes
Changes introduced in this pull request: