Conversation
victorlin
force-pushed
the
victorlin/organize-dockerfile
branch
from
73048a5 to
472f556
Compare
tsibley
approved these changes
Oct 13, 2022
Contributor
|
I extended the diff a bit to check the rest of the dirs. |
pip works, but use pip3 for consistency.
victorlin
force-pushed
the
victorlin/organize-dockerfile
branch
from
472f556 to
fecdb79
Compare
`X` only sets all execute bits if there is at least one already set. This isn't the case for some downloads, which is why it was set individually. Instead of setting individually, just use `x` to set all regardless of previous state.
These binaries are currently placed in the middle of pip installs. While they are relevant to the Pangolin packages, this deviates from existing organization of the Dockerfile which follows: 1. [builder] source builds 2. [builder] package manager installs 3. [final] direct downloads Add notes that these are for ncov/Pangolin. Also download them in a way that is consistent with other direct downloads. Since these commands are no longer bound by the CACHE_DATE declaration in the builder stage, they can take advantage of caching.
It's better to organize the Dockerfile for optimal version-pinning caching and keep direct downloads separate from package manager installs. This means it doesn't make sense to put the workflow-specific dependencies at the bottom. Instead, we can add searchable comments to each dependency to understand why it is included.
victorlin
force-pushed
the
victorlin/organize-dockerfile
branch
from
fecdb79 to
efcafe6
Compare
Use these sections to organize the order of adding software tools: 1. Built from source 2. Pre-built binaries 3. Installed via pip 4. Nextstrain components and unpinned software This means all software is managed in the builder stage. Use new directories /out/* to copy from builder stage to final stage. This also reduces the tight coupling of COPY --from=builder commands. Also, use `&&` to chain related commands together in the same RUN to reduce the number of layers.
victorlin
force-pushed
the
victorlin/organize-dockerfile
branch
from
8c3c27c to
a7159ec
Compare
Member
Author
|
Will merge once CI and the manual diff check passes. |
Member
Author
|
Updated diff check to ignore timestamps and use newer image: docker run nextstrain/base:build-20221013T234036Z ls -lR --time-style='+' /usr/local/{bin,libexec,share} > old.txt
docker run nextstrain/base:branch-victorlin-organize-dockerfile ls -lR --time-style='+' /usr/local/{bin,libexec,share} > new.txt
diff old.txt new.txt
# no diff! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
Some improvements to the Dockerfile.
Related issue(s)
CACHE_DATETesting
nextstrain/base:branch-victorlin-organize-dockerfile: