Just a friendly nudge in case there'll be time to ...
# development
c
Just a friendly nudge in case there'll be time to look at https://github.com/pantsbuild/pants/pull/13386 in the next few days.. ? ;) cc @happy-kitchen-89482 @hundreds-father-404
👀 1
h
Thank you for the ping! That's very useful for me, I sometimes miss things
👌 1
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
👍 1
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
?
h
Thanks for the nudge! Things have been hectic, but will try to get to this in the next couple of days.
👍 1
c
@hundreds-father-404
update the PR description with what we discussed for the motivation: really this change is about unblocking target generators for Dockerfiles
The PR already says:
In order to support building Docker images without a dedicated 
Dockerfile
 on disk, the 
dockerfile
 target has a codegen rule that creates the 
Dockerfile
 on demand.
It’s not clear to me what to update here..
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.
👍 1
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.
1
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..
Copy code
# 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")
h
I see what you're getting at, but I think it would be nicer to have less boilerplate. E.g., in the common case, where the dockerfile is in a file called "Dockerfile" in a directory, just
docker_image()
in that directory works. I don't expect
instructions
to be that commonly used, at least when adopting Pants, so I'd rather optimize for the common case.
👍 1
So why not put
instructions
and
source
directly on
docker_image
?
c
Yep, with my last input here, I kind of see your point, the
dockerfile
target as a stand alone construct doesn’t add much value any more.
1
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.
💯 1
h
We can make an OptionalSource field I think
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
👍 2
c
OK, I can look at a way to override the file not found behavour respect the
expected_num_files
on a source field.
https://github.com/pantsbuild/pants/issues/13567 See if that makes sense to you guys 🙂 Appreciate the iterations to get this polished.
👀 1
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
👍 1