ci: add workflow that lints shell scripts with ShellCheck #147
Conversation
Signed-off-by: Jack Baldry <[email protected]>
Signed-off-by: Jack Baldry <[email protected]>
I've not bothered saving this patch for reapplication if upstream changes because we had already deviated from that before these changes. - Use consistent variable syntax - Prefer `-n` over `! -z` - Use appropriate quoting for words that have no variable expansion Signed-off-by: Jack Baldry <[email protected]>
Signed-off-by: Jack Baldry <[email protected]>
- Remove useless echo subshell - Set readonly variables for to clarify when the script is no longer going to modify those variables. Signed-off-by: Jack Baldry <[email protected]>
https://grafana.com/docs/writers-toolkit/ - Simplify some language for improved readability - Prefer [semantic line breaks](https://sembr.org/) for better line based diffing in the GitHub UI. Signed-off-by: Jack Baldry <[email protected]>
Signed-off-by: Jack Baldry <[email protected]>
|
@jdbaldry the changes in the |
Signed-off-by: Jack Baldry <[email protected]>
jdbaldry
force-pushed
the
2024-06-shellcheck-shell-scripts
branch
from
ee6c13d
to
e90a068
Compare
|
I think the Test get-vault-secrets action / test (dev/ops) (pull_request) failures are due to my PR being in a fork. I don't seem to have write permissions to create a repository branch. I'm assuming Lint PR title / lint-pr-title (pull_request) is expecting some sort of conventional commit-like message in the PR subject so I'll try that but the error message "Error: subject may not be empty; type may not be empty" isn't very useful to someone new to the repository or to the idea of using conventional commit-like PR subjects. |
jdbaldry
changed the title
jdbaldry
changed the title
|
@dsotirakis Sorry for leaving this for so long. I've fixed the tests to verify that the behavior is unchanged after the linting fixes. |
|
Automated rebase attempt failed. Please rebase manually. |
|
I believe that any remaining CI failures are because I'm working in a fork. If I'm given write access to the repository, I'm happy to create a new branch and confirm there. |
I figure it'll be best if we can fix our CI to at least not fail for forks, so I've made an attempt in #472. |
There was a problem hiding this comment.
Great!
We could also consider encouraging a style guide for readability (perhaps starting with google.github.io/styleguide/shellguide.html).
Sounds good to have a followup with that. It's all great stuff, but I'd particularly appreciate us having some guidelines on when not to use shell scripts, and this guide has a section covering that topic.
This reverts commit 4278510.
Signed-off-by: Jack Baldry <[email protected]>
Signed-off-by: Jack Baldry <[email protected]>
I believe that ShellCheck is a great example of how linting can help share knowledge.
Each linting error is documented and the feedback it provides helps you write more understandable scripts with fewer bugs.
Separating shell scripts also helps with consistent formatting, makes modifications easier, and facilitates testing.
We could also consider encouraging a style guide for readability (perhaps starting with https://google.github.io/styleguide/shellguide.html).