-
Notifications
You must be signed in to change notification settings - Fork 81
Add Dockerfile to build docker image. #78
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
Conversation
Example use of the Docker support in pantsbuild/pants#12363 Signed-off-by: Andreas Stenius <andreas.stenius@svenskaspel.se>
@@ -23,6 +23,9 @@ resources( | |||
pex_binary( | |||
name="pex_binary", | |||
entry_point="main.py", | |||
|
|||
# needed when used in the Docker image, in case pants is running on a incompatible platform | |||
platforms=['linux-x86_64-cp-38-cp38'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to be able to "inject" or otherwise ensure that there is a specific value present for the platforms
field of a pex_binary
target.
I'm thinking that when packaging for the Docker build context, we could "know" what platform we want it packaged for..
Related pantsbuild/pants#13185.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that you can't run the binary anymore if you're on macOS, which is a show stopper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is possible to add more platforms there, so Mac would still be supported?
But my q is after if we could leave this change out entirely, and add it as needed when packaging for the docker image..
All hypothetical as we'll close this particular PR. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With adding automatically, how would we know which Python version to target? I don't think static analysis of the Dockerfile would work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think static analysis of the Dockerfile would work
No, that's problematic. I'd like a platform of just "linux-x86_64"
, possibly with the -cp
but not more than that.. hmm.
For specific cases, such when you use FROM python:3.9
for instance, we can, but that's very specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Andreas for adding this! I'm not sure we want to land it, though. We've gotten feedback that our example repo was too complex, hence #84 and #85
From there:
We would rather have the example repo be very easy to get started than to show every bell and whistle of Pants. Our docs are for the niche features like AWS Lambdas. Our example repo is to allow you to very easily try out Pants to get a feel for it before committing to prototyping in your own repo.
Now, Docker is interesting because it is much more used than Protobuf and AWS Lambdas are...I'm hesitant if we should land this, but I could see it being worth it.
Another option is to create an example-docker repo. It does increase maintenance burden because we have yet another repo to update etc, but it also frees you up to show off more relevant bells-and-whistles of Docker support whereas we otherwise would want this to be As Simple As Possible. For example, I think it could be neat to show off the tagging feature, but we probably wouldn't want to get that complex in example-python.
Wdyt? (This convo might be more conducive to Slack also if you want feedback from others)
@@ -23,6 +23,9 @@ resources( | |||
pex_binary( | |||
name="pex_binary", | |||
entry_point="main.py", | |||
|
|||
# needed when used in the Docker image, in case pants is running on a incompatible platform | |||
platforms=['linux-x86_64-cp-38-cp38'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that you can't run the binary anymore if you're on macOS, which is a show stopper.
Yeah, I came here today to close it, when that question I added here today popped out at me ;) +1 for a dedicated repo. It's becoming a rather feature intense backend, so good to be able to show various ways to do things. |
Closing this in favour of a dedicated |
Example use of the Docker support in pantsbuild/pants#12363
Signed-off-by: Andreas Stenius andreas.stenius@svenskaspel.se