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 CI job, fix/handle all shellcheck findings #620

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dbast
Copy link

@dbast dbast commented Nov 29, 2024

This fixes the shellcheck CircleCI job:

  • The existing job does nothing as the CI images does not support DIND (docker in docker), see error docker: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running? in https://app.circleci.com/pipelines/github/tilt-dev/tilt-extensions/1678/workflows/c1a75955-880d-408c-8188-cfe7651a9085/jobs/4065
  • That is why the shellcheck circle orb is used instead.
  • Even if that would work, it would still have always run green as the code in the Makefile target does not propagate all the errors.
  • The Makefile target is updated so that the errors propagate to the final exit code ... for local bug fixing, without running circle ci.
  • The Make target is further updated to use the official upstream container koalaman/shellcheck:stable, which is not 7 years old.

This PR additional fixes / handles all the previous not shown shellcheck findings, to get the noise down and the PR green -> CI becomes actionable feedback for new PRs.

@dbast dbast marked this pull request as draft November 29, 2024 15:48
@dbast dbast marked this pull request as ready for review November 29, 2024 15:52
@dbast dbast marked this pull request as draft November 29, 2024 15:53
Signed-off-by: Daniel Bast <[email protected]>
@dbast dbast force-pushed the fix-shellcheck branch 21 times, most recently from 41062ac to a43e791 Compare November 29, 2024 21:44
@dbast dbast marked this pull request as ready for review November 29, 2024 21:51
@dbast
Copy link
Author

dbast commented Nov 29, 2024

yay. finally green after several iterations. Ready for review. thanks!

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.

1 participant