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

Allow ARGs in COPY --from and RUN --mount=from #3551

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Jan 30, 2023

🛠️ Fixes #2717 (#2412 is also relevant)

Previously, using ARGs in COPY --from or RUN --mounts from was not supported, requiring using an extra intermediate stage. This patch allows global args to be used directly in these locations.

To do this, we need to resolve a dependency issue:

  • We need to construct all the dispatchStates before evaluating.
  • To construct the dispatchStates, we need to determine the fully-evaluated base LLB source.
  • However, to fully-evaluate the base name using the Expand interface, we need to have constructed all the dispatchStates.

It is theoretically possible to refactor to not have this restriction, however, this is not required to implement the base functionality for global args.

To resolve this, we can use just the global args to evaluate the FROM fields using the toCommand helper, where the stage resolution occurs today.

At some point, it should be possible to implement local arg resolution as well here - however, this requires a change from the current approach of creating an intermediate dispatchState for those components.

@tonistiigi does this seem like a reasonable approach for a first pass?

Previously, using ARGs in COPY --from or RUN --mounts from was not
supported, requiring using an extra intermediate stage.  This patch
allows global args to be used directly in these locations.

To do this, we need to resolve a dependency issue:
- We need to construct all the dispatchStates before evaluating.
- To construct the dispatchStates, we need to determine the
  fully-evaluated base LLB source.
- However, to fully-evaluate the base name using the Expand interface,
  we need to have constructed all the dispatchStates.

It is theoretically possible to refactor to not have this restriction,
however, this is not required to implement the base functionality for
global args.

To resolve this, we can use just the global args to evaluate the FROM
fields using the toCommand helper, where the stage resolution occurs
today.

Signed-off-by: Justin Chadwell <[email protected]>
@jedevc jedevc requested a review from tonistiigi January 30, 2023 14:03
@thaJeztah
Copy link
Member

I think some of the hesitation at the time was that the UX could be confusing. For example;

ARG HELLO=world

FROM alpine
ENV HELLO=something
RUN echo $HELLO
COPY --from=$HELLO,...

In this, what value would be used for $HELLO? (in both cases)? (Probably same for cases where an ARG HELLO would be set in build-stage (FROM))

@tonistiigi
Copy link
Member

tonistiigi commented Jan 31, 2023

This patch allows global args to be used directly in these locations.

I can see how this can be quite confusing that some mount values have different ARG values than others. If it is global, then we could just detect these cases and show a warning or error instead. Using the intermediate stage isn't hard, but it isn't obvious to a new user that they need to do it.

edit: looking at the code, I had forgotten that we already are covering this error case.

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.

Support --from=$var syntax
3 participants