search

tarides / opam-monorepo

Assemble dune workspaces to build your project and its dependencies as a whole
ISC License
130 stars 27 forks source link

Fix for Dockerfile smell DL3059 #382

Closed grosa1 closed 1 year ago

grosa1 commented 1 year ago

Hi! The Dockerfile placed at "Dockerfile.test" contains the best practice violation DL3059 detected by the hadolint tool.

The smell DL3059 occurs if there are multiple consecutive RUN instructions. Each RUN will correspond to a layer of the final Docker image, thus is recommended to compact them to reduce the number of layers for the final image. This pull request proposes a fix for that smell generated by my fixing tool. The patch was manually verified before opening the pull request. To fix this smell, specifically, The consecutive RUN instructions have been chained until a comment or a different instruction appears.

This change is only aimed at fixing that specific smell. If the fix is not valid or useful, please briefly indicate the reason and suggestions for possible improvements.

Thanks in advance

Leonidas-from-XIV commented 1 year ago

Well that particular patch won't work because the syntax is wrong. --mount=type=cache,target=/home/opam/.cache,uid=1000 are arguments to RUN, not commands to be executed so if contracting the commands via && they'd need to be remove.

grosa1 commented 1 year ago

Oh sorry, I didn't notice that. This is a false positive that should be excluded. Thank you for the response!

Leonidas-from-XIV commented 1 year ago

It can probably still be improved by joining those but given the file is only for testing purposes the additional layer of caching could be useful.

OTOH, this dockerfile is so old and outdated, it still uses the old name of the binary and doesn't work anyway. Maybe we should just remove it instead.