-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Create docker_environment
from docker_image
#17714
Comments
This would be a really helpful feature. Ideally, with an option for specifying |
I believe I've got a proof-of-concept on this after a ton of cyclic dependencies, and then cyclic rule graphs errors 😆 As per Stu's recommendation in Slack:
I kinda took the approach of just doing this. Allow Before I can PR: Are there any huge piles I've just stepped into? I'm hoping not as this isn't a transitive dep from the environment. The quirk I've run into is needing to "escape" the What would be a decent API as a consumer? The "easy" answer is using the In this case, the docker_image(
name="my-docker-image".
instructions="..."
)
docker_environment(
name="my-docker-env",
dependencies=[":my-docker-image", ...], # <-- There can technically be multiple, and we pre-build all of them - I think we'd want to enforce these are explicitly `docker_image`s
image="my-docker-image", # <-- this gets replaced with the SHA256 at runtime? That might still do a pull from the registry, need to look into it and `bollard` - needs some thought on implementation still
) Alternative, overload the docker_image(
name="my-docker-image".
instructions="..."
)
docker_environment(
name="my-docker-env",
image=":my-docker-image", # <-- This is built and replaced at runtime - this feels the most Pantsish. Could be a problem if Docker images allow a colon or slash prefix in their labels, need to check tonight
) Lastly, we could add a new field like |
I would need to see the draft PR before I could opine on what piles have actually been stepped in. (E.g., I don't have enough context to guess at what the cyclical dependencies were.) |
I've not much to say about the cyclic dependencies, other than GLHF ;) |
What would be the purpose of allowing non-docker deps if we wouldn't use them in the environment? I think if we ever did want to use them, we could remove the limitation 🤷🏽 My API usage assumption is that any dependency in an environment MUST end up available in that environment - which wouldn't be the case here
Making one dependency special by order, I think, is a bit risky to me - might end up with some really weird behaviour. I also don't think it's a known convention everywhere in Pants. Building on your ideas though, what about:
The place where I could see this get weird is like, what if a user does this: docker_image(
name="my-docker-image"
)
docker_environment(
name="my-docker-environment",
dependencies=[":my-docker-image", ...],
image="nginx:latest"
) Nothing wrong with this technically, but to me, this feels like a mistake, no? The more I think about it, this makes the most sense from the API and keeps everything obvious/explicit, but it's not as extensible technically (but, like, should it be? Do we have a use case for hijacking a lazily instantiated docker_environment(
name="my-docker-environment",
image="//:my-docker-image"
) One benefit of this way is that, it seems like something we'd want to have eventually anyways (as per my above case, of specifying a single item in a list of In fact, the ONLY problem I can see is if |
Requiring a target address be rooted with |
Is your feature request related to a problem? Please describe.
I would like to use a custom docker image generated using
docker_image
for creating adocker_environment
.Describe the solution you'd like
In the
docker_environment
setname
= <docker_image_target_name>Describe alternatives you've considered
I didn't know how it would work. But as mentioned by @stuhood on slack
Additional context
Doc
slack thread
The text was updated successfully, but these errors were encountered: