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

Feature/clean dockerfile #14

Merged
merged 8 commits into from
Nov 28, 2023
Merged

Feature/clean dockerfile #14

merged 8 commits into from
Nov 28, 2023

Conversation

AlexAxthelm
Copy link
Contributor

Some general cleaning of the Dockerfile.

Notable Changes:

  • Consolidating R related RUN commands
  • Moving system dependencies out of an ARG, and explicitly calling them in RUN (eliminates source of error in passing a bad build arg)
  • Pinning System dependency versions
  • Not installing system dependencies through PAK, only through apt-get
  • Adding pandoc (required via rmarkdown for pacta.portfolio.utils) and libicu-dev (via stringi for pacta.portfolio.import and .allocate) to sysdeps list
  • remove --platform in FROM (in favor of specifying platform in build command, see Use docker offical build action #12 )

merging `R` related RUN commands
resolves hadolint 3059: https://github.com/hadolint/hadolint/wiki/DL3059
Hard code system dependencies into Dockerfile to avoid changing during
build process.
It is better to install them ourselves so we know what is being included
Adding:

* libicu-dev (needed by `stringi`)
* `pandoc` (needed by `knitr` & `rmarkdown`)
@AlexAxthelm AlexAxthelm requested a review from cjyetman November 16, 2023 22:55
Copy link

github-actions bot commented Nov 16, 2023

Docker image from this PR (c726bb7) created

docker pull ghcr.io/rmi-pacta/workflow.pacta:pr14

@AlexAxthelm
Copy link
Contributor Author

We can get rid of pandoc if we resolve RMI-PACTA/pacta.portfolio.utils#16

This was referenced Nov 17, 2023
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@AlexAxthelm AlexAxthelm requested a review from cjyetman November 28, 2023 09:16
Copy link
Member

@cjyetman cjyetman left a comment

Choose a reason for hiding this comment

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

approved with very strong reservations as stated in the comments

@AlexAxthelm AlexAxthelm merged commit 21c4aa8 into main Nov 28, 2023
4 checks passed
@AlexAxthelm AlexAxthelm deleted the feature/clean-dockerfile branch November 28, 2023 10:32
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