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

[WIP] Hadolint recommendations #41

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TokisanGames
Copy link
Contributor

@TokisanGames TokisanGames commented Mar 19, 2020

I ran http://github.com/hadolint/hadolint per calinou. This resulted in 45 best practices warnings. I fixed all except for using WORKDIR instead of cd, and pinning apt-get installs (and dnf) to specific versions).

DL3003 Use WORKDIR to switch to a directory
DL3008 Pin versions in apt get install. Instead of `apt-get install <package>` use `apt-get install <package>=<version>`

Key takeaways:

  • Any docker that uses | should have SHELL ["/bin/bash", "-o", "pipefail", "-c"] before RUN so it will fail if either side of the pipe fails.
  • Globs *.deb should begin with ./*.deb in case a file begins with a hyphen so it's not interpreted as an option.
  • -e on echo is not a POSIX standard and printf is recommended.
  • CMD is a json list, so should be formatted as CMD ["/bin/bash"]
  • Because of the above, I combined xcode RUN and CMD into one command and removed the redundant line in build.sh


CMD mkdir -p /root/xcode && \
clang -O3 -llzma -lxar -I /usr/local/include pbzx.c -o pbzx && \
mkdir -p /root/xcode && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merge into run command. Separate CMD w/ json format

ln -sf /usr/bin/g++-8 /usr/bin/g++ && \
apt-get clean && \
rm -rf /var/lib/apt/lists/ && \
rm ./*.deb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clean up files on the layer that installs them

@@ -33,11 +38,8 @@ RUN cp -a /root/files/${mono_version} /root && \
wget https://download.mono-project.com/repo/ubuntu/pool/main/c/core-setup/msbuild-libhostfxr_3.0.0.2019.04.16.02.13-0xamarin3+ubuntu1604b1_i386.deb && \
wget https://download.mono-project.com/repo/ubuntu/pool/main/m/msbuild/msbuild-sdkresolver_16.3+xamarinxplat.2019.08.08.00.55-0xamarin2+ubuntu1604b1_all.deb && \
wget https://download.mono-project.com/repo/ubuntu/pool/main/n/nuget/nuget_5.2.0.6090.bin-0xamarin1+ubuntu1604b1_all.deb && \
dpkg -i --force-all *.deb && \
dpkg -i --force-all ./*.deb && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

avoid files that begin with - from being interpreted as an option

@@ -3,7 +3,9 @@ FROM godot-mono:${img_version}

ARG mono_version

RUN if [ -z "${mono_version}" ]; then echo -e "\n\nargument mono-version is mandatory!\n\n"; exit 1; fi && \
SHELL ["/bin/bash", "-o", "pipefail", "-c"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

script stops if either side of a pipe fail

@TokisanGames TokisanGames changed the title Hadolint recommendations [WIP] Hadolint recommendations Mar 25, 2020
@TokisanGames
Copy link
Contributor Author

In spite of what hadolint says, SHELL ["/bin/bash", "-o", "pipefail", "-c"] is apparently not accepted by podman. Though it doesn't cause any errors building the docker, it does report this message:

time="2020-03-25T15:53:41+08:00" level=error msg="SHELL is not supported for OCI image format, [/bin/bash -o pipefail -c] will be ignored. Must use `docker` format"

Making this one WIP while I figure it out.

@akien-mga
Copy link
Member

This should be rebased to remove the commits from #40 (merged) and #42 (not merged yet but I will after branching the 3.2 buildsystem off).

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.

2 participants