Skip to content

fix: convert from long RUN statements to a setup.sh script - #173

Merged
JohnVillalovos merged 2 commits into
masterfrom
jlvillal/remove_shell
Mar 10, 2026
Merged

fix: convert from long RUN statements to a setup.sh script#173
JohnVillalovos merged 2 commits into
masterfrom
jlvillal/remove_shell

Conversation

@JohnVillalovos

@JohnVillalovos JohnVillalovos commented Mar 8, 2026

Copy link
Copy Markdown
Collaborator

Convert the multiple RUN statements into a setup.sh bash script.

  • Add a CI job to check the shell scripts.
  • Resolve shellcheck issues with bin/*.sh
  • Run shfmt -i 2 -ci -w bin/*sh

Copilot AI review requested due to automatic review settings March 8, 2026 14:50
@JohnVillalovos
JohnVillalovos marked this pull request as draft March 8, 2026 14:50
@JohnVillalovos
JohnVillalovos force-pushed the jlvillal/remove_shell branch from 1f0e1e5 to fb24c4c Compare March 8, 2026 14:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes the Dockerfile-level SHELL override (to avoid relying on a non-OCI instruction) and instead makes the heredoc RUN blocks explicitly run under bash, preserving pipefail behavior within those scripts.

Changes:

  • Removed SHELL ["/bin/bash", "-o", "pipefail", "-c"] from the Dockerfile.
  • Updated heredoc RUN blocks to invoke bash directly and set -o pipefail via set -xeuo pipefail.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread Dockerfile Outdated
@JohnVillalovos
JohnVillalovos force-pushed the jlvillal/remove_shell branch 9 times, most recently from b8e890a to d350b05 Compare March 8, 2026 15:53
@JohnVillalovos
JohnVillalovos requested a review from Copilot March 8, 2026 15:54
@JohnVillalovos JohnVillalovos changed the title fix: remove use of SHELL and specify bash in RUN fix: convert from long RUN statements to a setup.sh script Mar 8, 2026
@JohnVillalovos
JohnVillalovos marked this pull request as ready for review March 8, 2026 15:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread Dockerfile Outdated
Comment thread Dockerfile
@JohnVillalovos

JohnVillalovos commented Mar 8, 2026

Copy link
Copy Markdown
Collaborator Author

@colisee Do you prefer 2 space indents in bash files? I usually use 4, but I notice 'shfmt -i 4' caused a lot of changes.

Since most (but not all) was using 2 space indents I did that. And added a header to the files so that editors would know.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread setup.sh Outdated
Comment thread setup.sh

@ikke-t ikke-t left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume apart from linter and while loop and dockerfile all are just white space changes?

I didn't test these, but read thrm through. Nothing apart removing setup.sh from container catches my eye.

Comment thread Dockerfile
@JohnVillalovos
JohnVillalovos force-pushed the jlvillal/remove_shell branch 2 times, most recently from 673cd5f to eb82df2 Compare March 8, 2026 19:25
@JohnVillalovos

Copy link
Copy Markdown
Collaborator Author

I have updated the PR and made shfmt use an indentation level of 2, as that is what was there before. So the diff is now simpler.

I assume apart from linter and while loop and dockerfile all are just white space changes?

Yes. That is correct.

Nothing apart removing setup.sh from container catches my eye.

Done

@JohnVillalovos
JohnVillalovos force-pushed the jlvillal/remove_shell branch from eb82df2 to 33977eb Compare March 8, 2026 19:38
@JohnVillalovos
JohnVillalovos requested a review from ikke-t March 8, 2026 19:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread setup.sh
Comment thread .github/workflows/build_pull_request.yml
Comment thread .github/workflows/build_pull_request.yml
Comment thread Dockerfile
Comment thread setup.sh
Comment thread setup.sh
Comment thread setup.sh
@colisee

colisee commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Aside from your comments, above, I tested a build with podman and ran a podman kube play and it worked like a charm.

@colisee
colisee self-requested a review March 10, 2026 07:26
@JohnVillalovos
JohnVillalovos force-pushed the jlvillal/remove_shell branch from 1324de3 to 23e4886 Compare March 10, 2026 15:01
Convert the multiple `RUN` statements into a `setup.sh` bash script.

Add a CI job to check the shell scripts.
Also run `shfmt -i 4 -ci -w bin/*sh`
@JohnVillalovos
JohnVillalovos force-pushed the jlvillal/remove_shell branch from 23e4886 to 9b404d8 Compare March 10, 2026 19:51
@JohnVillalovos
JohnVillalovos merged commit e3f875d into master Mar 10, 2026
3 checks passed
@JohnVillalovos
JohnVillalovos deleted the jlvillal/remove_shell branch April 30, 2026 14:17
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.

4 participants