Allow to use checksum in ADD from ENV #3287
Conversation
| ENV DIGEST=%s | ||
| ENV LINK=%s | ||
| ADD --checksum=${DIGEST} ${LINK} /tmp/foo |
There was a problem hiding this comment.
using environment variables and adjust them using some logic around.
I'm a bit confused by this part, as both ENV and ADD --checksum=<some_env> are hard-coded in the Dockerfile, or was that just to avoid having to include the digest twice?
I seem to recall we added substitution for some other flags; were those flags accepting ENV, ARG (or both?)
There was a problem hiding this comment.
We should probably also update the documentation to mention that variable substitution is supported for this option (https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#verifying-a-remote-file-checksum-add---checksumchecksum-http-src-dest)
There was a problem hiding this comment.
I'm a bit confused by this part, as both
ENVandADD --checksum=<some_env>are hard-coded in the Dockerfile, or was that just to avoid having to include the digest twice?
It is just small sample I use as test here. Having digest as an ENV will make Dockefile looks better as we will have all ENVs in one place where we put the link, the checksum (version, options, something else...). Also in case of complex behavior when we use different versions (different links) for different target arches we should have control on checksum field, so we should use variables.
For example:
FROM foo-build-base AS foo-build-amd64
ENV FOO_VERSION=2
ENV FOO_DIGEST=sha256:...
ENV FOO_OPTIONS=...
ENV FOO_PATCHES=...
FROM foo-build-base AS foo-build-arm64
ENV FOO_VERSION=1
ENV FOO_DIGEST=sha256:...
ENV FOO_OPTIONS=...
ENV FOO_PATCHES=...
FROM foo-build-${TARGETARCH} AS foo-build
ADD --checksum=${FOO_DIGEST} ${FOO_LINK}/${FOO_VERSION}.tar.gz /foo.tar.gz
RUN build_foo...
Do you want me to modify the test somehow?
There was a problem hiding this comment.
We should probably also update the documentation to mention that variable substitution is supported for this option (https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#verifying-a-remote-file-checksum-add---checksumchecksum-http-src-dest)
It seems to me that this is a separate PR with improvements in the documentation. I can see a general mention of substitution support for the ADD command here: https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#environment-replacement. But going into details, we expand source, destination, chown, but they are not mentioned either. And mentioning that the checksum field supports substitution would give the wrong impression (at least to me) that other fields don't.
| @@ -254,6 +253,12 @@ func (c *AddCommand) Expand(expander SingleWordExpander) error { | |||
| } | |||
| c.Chown = expandedChown | |||
|
|
|||
| expandedChecksum, err := expander(c.Checksum) | |||
There was a problem hiding this comment.
I think it is better if you keep the Checksum as digest.Digest. In here after expansion, you should call digest.Parse() to convert the string to Digest and error if the value is invalid.
There was a problem hiding this comment.
Unfortunately it is not possible to store AddCommand's Checksum as digest.Digest (I can only try to do validation without type cast inside Expand, but it looks quite misplaced) as we set it before expansion. Sorry if I misunderstood your suggestion.
I modified the code and now convert (and check) during filling of copyConfig.
We cannot use checksum from environment variables (for example with
`ADD --checksum=${DIGEST} ${LINK} /tmp/foo`), which made this
option useless in cases when we define url using environment variables
and adjust them using some logic around.
Let's add checksum field to Expand function to fill it before sending
out.
Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
giggsoff
force-pushed
the
fill-options-from-envs
branch
from
b31ada2
to
be3db58
Compare
We cannot use checksum from environment variables (for example with
ADD --checksum=${HASH} ${LINK} /tmp/foo), which made this option useless in cases when we define url using environment variables and adjust them using some logic around.Let's add checksum field to Expand function to fill it before sending out.
PR contains implementation and test