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 Shellcheck violations #125

Merged
merged 17 commits into from Jan 16, 2021

Conversation

Potherca
Copy link
Member

This MR fixes a lot of the violations mentioned in #78 and adds a GitHub Action to prevent regression.

All of the following files are now clean:

  • lib/getdeps/getdeps.sh
  • lib/list/list.sh
  • lib/package/package.sh
  • lib/show/show.sh
  • lib/term/term.sh
  • lib/update/update.sh
  • lib/utils/utils.sh

The output of the GitHub Action can be seen here: https://github.com/potherca-contrib/bpkg/actions/runs/475635343

This was referenced Jan 10, 2021
@szepeviktor
Copy link

You can use checkstyle output format and convert it to GitHub annotations.
https://github.com/staabm/annotate-pull-request-from-checkstyle

@szepeviktor
Copy link

No!
Look at this: https://github.com/marketplace/actions/shellcheck

@Potherca
Copy link
Member Author

Potherca commented Jan 10, 2021

@szepeviktor I'd rather use actions/pipeline-components-shellcheck than actions/shellcheck, as action-shellcheck does work in the host, where the pipeline-component is a self-contained docker image.

Because of this, the action is guaranteed to have fewer issues when run locally (using act). It also makes it possible to use the same image on other systems (either other CI or locally, if act is not present/desirable).

staabm/annotate-pull-request-from-checkstyle looks overly complex, considering that all you need are a problem matcher json and an echo ::add-matcher:: call (as demonstrated in https://github.com/potherca-contrib/bpkg/pull/2/files)

We're in the process of adding problem matchers for GHA to all the Pipeline-Components, so only one action would be needed for both running shellcheck and annotating the MR.

@Potherca
Copy link
Member Author

@jwerle I'm inclined to merge this to develop rather than master so it can be beta-tested before being released. (Since people seem to be pulling from master rather than using provided versions...).

@jwerle
Copy link
Member

jwerle commented Jan 16, 2021

Good idea. Yeah, that sounds good

@Potherca Potherca changed the base branch from master to develop January 16, 2021 17:06
@Potherca Potherca merged commit 716fc7d into bpkg:develop Jan 16, 2021
@Potherca Potherca deleted the fix/shellcheck-violations branch January 16, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants