Might be helpful to update the PR description with what we discussed for the motivation: really this change is about unblocking target generators for Dockerfiles

It would also be helpful to mention the alternative designs considered

Hm so it looks like you're suggesting we go with Option B? A

dockerfile

target that gets coupled to a

docker_image

target? That was my least favorite option because of the coupling, but willing to keep an open mind Question: would you anticipate it every not being a 1:1 relationship? E.g. that multiple

docker_image

targets use the same

dockerfile

?

you’re suggesting we go with Option B? A 

dockerfile

 target that gets coupled to a 

docker_image

 target?

Yes, that is what I (and [also Benjy](https://github.com/pantsbuild/pants/pull/13386#discussion_r742223057) ) suggested.

That was my least favorite option because of the coupling

I don’t really see what’s bad with this coupling.. ? From my point of view, the

dockerfile

target represents its own thing, the “Dockerfile”. When it does not exist on disk so we can reference it with the

source

field of the

docker_image

we can create one with the codegen target instead and add it as a dependency. The

docker_image

target describes our image, not the Dockerfile. That’s my main motivation for not simply introducing the

instructions

field directly on the

docker_image

target. (Which would be option C).

Q: would you anticipate it every not being a 1:1 relationship? E.g. that multiple 

docker_image

 targets use the same 

dockerfile

?

That may very well be the case. Not something that I’ve designed for, but as you may write a generic Dockerfile using build args, you could certainly use the same

dockerfile

with multiple

docker_image

targets with different build arguments.

There’s one thing that I’m not super fond of with option B, and that is that we don’t get the default source, so we need the

source="Dockerfile"

when using a file from the workspace. Would love to solve this somehow.. but introducing another target next to

docker_image

solely for this reason feels a bit too much, for me. (For reasons outlined above, being that it is not an alternative for

docker_image

we need, but for the

Dockerfile

). Hence why it is really hard to come up with a sensible name for this secondary target.

OK, I think I may have come up with a solution that will allow us to still keep the default behaviour of looking for a Dockerfile in the workspace. See if this feels like an OK approach.. We make

dockerfile

support using either the

instructions

field to generate the Dockerfile or use the

source

field to use a file from disk. With this addition, a

docker_image

target may generate a

dockerfile(source="Dockerfile")

target and inject a dependency on that, in order to preserve the default behaviour. It may feel a bit clumsy, but this should all be implementation details. In case there is no “Dockerfile” on disk, the error message will be slightly different from when the default was from the source field on the docker_image target, but that would be all. In the end, I think I like the idea of promoting the Dockerfile to a distinct target to work with, separate from the

docker_image

target.

To illustrate the above..

# still supported
docker_image(source="CustomDockerfile", ...)

# explicit dep
docker_image(dependencies=[":dockerfile_tgt"], ...)

# either of these
dockerfile(name="dockerfile_tgt", instructions=[...])
dockerfile(name="dockerfile_tgt", source="CustomDockerfile")

# Default
docker_image(
  name="MyImage",

  # injected dep
  dependencies=[":MyImage#Dockerfile"]
)

# generated target by docker_image
dockerfile(name="MyImage#Dockerfile", source="Dockerfile")

So why not put

instructions

and

source

directly on

docker_image

?

But there’d still be the issue with the default value for

source

in that case.. Or, we simply require to set

source=""

if using the

instructions

field..?

I agree with optimising for the common case.

Or something

I don't want to impose on the user just because we once decided that a source field can't be optional... We can change that

https://github.com/pantsbuild/pants/issues/13567 See if that makes sense to you guys 🙂 Appreciate the iterations to get this polished.

PR https://github.com/pantsbuild/pants/pull/13578

Will update the dockerfile PR once 13578 has landed.

OK, I’ve updated the PR title and description after adapting the optional default glob ft introduced with 13578, and think I’ve got all the bits and pieces in place now finally. Thank you for the patience iterating on this. https://github.com/pantsbuild/pants/pull/13386