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

dockerfile: expose TARGETSTAGE as builtin argument #5431

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

tonistiigi
Copy link
Member

closes docker/buildx#2646

Allows capturing what build stage is currently being built for patterns with better base stage reuse.

@thaJeztah
Copy link
Member

This probably needs documentation changes to add this to the list of builtin args? 😅

bp := po.buildPlatforms[0]
tp := po.targetPlatform
if target == "" {
target = "default"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should have default if target not set. I think best would be to leave it empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Empty value could cause some breakage maybe. We do name stage default in the --call=targets output as well. Another option would be to use numbers as we still support them for access, but might be quite confusing.

Copy link
Member

Choose a reason for hiding this comment

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

We do name stage default in the --call=targets output as well.

Ah I forgot about this subrequest, so yeah default is good. I was just wondering if default could be used as TARGETSTAGE but that would not make sense.

Copy link

@ByteNybbler ByteNybbler Oct 18, 2024

Choose a reason for hiding this comment

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

I could see it going either way.

--call=targets annotates whatever the final build stage is with (default), regardless of whether the build stage's name is an empty string or not.

I think having TARGETSTAGE be default when the Dockerfile's targeted build stage doesn't have a name might help avoid some potentially surprising behaviors. For example, there could be a scenario where the value of TARGETSTAGE ends up getting used as a directory name.

On the other hand, a Dockerfile can have a build stage named default which isn't the final build stage, so applying a default value for TARGETSTAGE here might be surprising in its own way. But that sounds like a pretty unusual Dockerfile.

I think just going with default will be applicable to the vast majority of use cases.

frontend/dockerfile/dockerfile_test.go Show resolved Hide resolved
@tonistiigi
Copy link
Member Author

@ByteNybbler, any input on the comments above?

@ByteNybbler
Copy link

Yes, thank you so much for working on this! I'm very excited to have access to this feature!

@thaJeztah
Copy link
Member

Would setting a value by default (if not set) make it harder to do constructs where the user wants to set a default value? echo ${TARGETSTAGE:-development} (or similar constructs)

@tonistiigi
Copy link
Member Author

Would setting a value by default (if not set) make it harder to do constructs where the user wants to set a default value? echo ${TARGETSTAGE:-development} (or similar constructs)

Yeah, but if you look at the current TARGET* BUILD* default args then they are also always guaranteed to have a defined value. So I think it is more consistent this way, where it is always set by the builder and not left for the user to initialize in any case.

@ByteNybbler
Copy link

The only time TARGETSTAGE could ever use some possible default value (at least for how I'm imagining this will work) would be when the final build stage is unnamed in the Dockerfile. If the user wants to control the "default" value of TARGETSTAGE, they can do that by giving the final build stage an appropriate name in the Dockerfile, since the final build stage is what docker build targets by default when no --target option is provided. TARGETSTAGE should match the name of that target.

Allows capturing what build stage is currently being
built for patterns with better base stage reuse.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Member Author

Updated to read the default stage name from Dockerfile if not passed by the user.

@crazy-max crazy-max added this to the v0.17.0 milestone Oct 25, 2024
@tonistiigi tonistiigi merged commit bc6f7be into moby:master Oct 28, 2024
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access value of buildx build --target within Dockerfile
4 participants