Dockerfile cleanup #2861
Dockerfile cleanup #2861
Conversation
- don't cache packages to lighten the image.
Dockerfile
Outdated
| # Install Opam dependencies | ||
| ADD ocamlorg.opam ocamlorg.opam | ||
| # Install opam dependencies | ||
| COPY --chown=opam --link ocamlorg.opam . |
There was a problem hiding this comment.
Can we keep the old thing here?
ADD ocamlorg.opam ocamlorg.opam
There was a problem hiding this comment.
Ha, my mistake, I shouldn't have introduced the --link here, but COPY is preferable to ADD: Dockerfile best practice: ADD or COPY.
|
|
||
| # Branch freeze was opam-repo HEAD at the time of commit | ||
| RUN cd opam-repository && git pull origin c45f5bab71d3589f41f9603daca5acad14df0ab0 && opam update | ||
| RUN cd ~/opam-repository && git fetch -q origin master && git reset --hard c45f5bab71d3589f41f9603daca5acad14df0ab0 && opam update |
There was a problem hiding this comment.
This has always confused me. I think we've changed this line a couple of times. Back and forth between what we have now and what you're suggesting. Your line downloads some more doesn't it? Is there another difference?
There was a problem hiding this comment.
In my view, here, to avoid docker cache miss, we should just do git checkout to a hash that is known to be already in opam-repo history.
There was a problem hiding this comment.
I don't think it downloads more, as per the docs:
More precisely,
git pullrunsgit fetchwith the given parameters and then depending on configuration options or command line flags, will call eithergit rebaseorgit mergeto reconcile diverging branches.
So here by splitting git pull into git fetch and git reset, we're avoiding conflict resolution, though it's not clear to me why there would be any conflicts in the first place. Corruption could happen.
The base image already ships the opam repository, I tend to set this hash to the one already present in the image, so that the git reset operation is mostly free. It can be needed from time to time to update to a newer opam-repository, or just set it explicitly for reproducibility/debugging purposes. The base images are updated weekly, I don't know how often the deployed image is.
There was a problem hiding this comment.
The deployed image is updated fairly frequently. Several times a week, often more. If we can take advantage of this, it would be nice. That's why we are considering using a docker image pin instead of an opam repo pin
There was a problem hiding this comment.
Both may be needed, as the docker image is updated weekly, you might want to rely on a dependency that's been released during the week but not yet available in the base image.
There was a problem hiding this comment.
No we don't want to do that :-) We're laggards. We want to use old stuff that has been around for some time, preferably a long time. We run away from anything new, fresh, fancy or cool :-)
There was a problem hiding this comment.
There might be new, fresh, fancy, or cool bug fixes!
| RUN cd opam-repository && git checkout master && git pull origin master && opam update | ||
| ENV OCAMLORG_REPO_PATH opam-repository | ||
| ENV OCAMLORG_PKG_STATE_PATH package.state | ||
| RUN cd ~/opam-repository && git checkout master && opam update |
There was a problem hiding this comment.
Agreed, if you pulled in the previous stage, you don't need to do it again.
|
@MisterDA can you have a look at this? |
Co-authored-by: Antonin Décimo <[email protected]>
Co-authored-by: Antonin Décimo <[email protected]>
|
Terrible mistake I shouldn't have merged this! |
This reverts commit 620b813.
This reverts commit 620b813. Co-authored-by: Cuihtlauac ALVARADO <[email protected]>
* Dockerfile: fix style - don't cache packages to lighten the image. * Update Dockerfile Co-authored-by: Antonin Décimo <[email protected]> * Update Dockerfile Co-authored-by: Antonin Décimo <[email protected]> * Don't fetch before building the switch * Remove unwanted line --------- Co-authored-by: Antonin Décimo <[email protected]> Co-authored-by: Antonin Décimo <[email protected]> Co-authored-by: Cuihtlauac ALVARADO <[email protected]>
Extracted from PR #2831