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

make oci_image() accept a file for workdir #673

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lazcamus
Copy link

This clones much of the same logic as entrypoint and friends. Also refactor docstring a little to illuminate "wrapper vs the rule" and the dual typed args.

This clones much of the same logic as `entrypoint` and friends. Also refactor
docstring a little to illuminate "wrapper vs the rule" and the dual typed args.
@lazcamus
Copy link
Author

I used this to clone the language rule's "set a default workdir for language targets" logic from rules_docker

My intent is to extract some of my local changes and add default entrypoint and workdir setting to https://github.com/aspect-build/bazel-examples/tree/main/oci_python_image

@lazcamus lazcamus changed the title make the oci_image() macro wrapper accept a file for workdir make oci_image() accept a file for workdir Aug 16, 2024
@lazcamus
Copy link
Author

also worth noting that it breaks backwards compatibility: workdir = "/root" has to change to workdir = ["/root"]

@thesayyn
Copy link
Collaborator

I don’t think it makes sense to have an attribute that takes an array for a single value. Doesn’t make sense from public api standpoint.

@jjmaestro
Copy link
Contributor

@lazcamus I love the idea to be able to use the output of a target by just passing the label but I agree with @thesayyn re. not using an array. Also, per the Dockerfile spec, WORKDIR only has a single argument. IMHO it maps much better to just one string.

Following your notation (which I also think is great and clarifies the wrapper-vs-rule) I'd prefer if workdir could be STR_OR_LABEL. I think that brings in all the goodies from your PR and still maintains a more natural API that's aligned with the Dockerfile spec.

@lazcamus
Copy link
Author

lazcamus commented Sep 1, 2024 via email

It kinda doesn't exist in macro-land, so build a hacky string parsing heuristic
@lazcamus
Copy link
Author

lazcamus commented Sep 2, 2024

I pushed a commit that makes workdir a STRING_OR_LABEL, so PTAL

The reason I originally ended up on the list is b/c it's easy to check with types in a macro, and everybody else was using lists so it kinda blended in 😆

There is no "is this string a valid label?" check in macro context, so I did some hacky string matching.

It comes with the corner case of: what do you do if you get "foo" ? It's a valid target (I'd use ":foo" but not everybody is me). "foo" is also a valid relative workdir (I also only use absolute workdirs). I chose label for this weird in-between case, but could see it either way. A workaround if you want "foo" to be a relative workdir string is to use "./foo", which seems OK 🤷

@alexeagle alexeagle added this to the 2.0 milestone Sep 5, 2024
@lazcamus
Copy link
Author

lazcamus commented Sep 6, 2024

CI status seems like a runner failure, hopefully intermittent?

ERROR: /home/runner/work/rules_oci/rules_oci/examples/dockerfile/BUILD.bazel:18:11: BuildDocker examples/dockerfile/base [for tool] failed: (Exit 1): buildx failed: error executing BuildDocker command (from target //examples/dockerfile:base) bazel-out/k8-opt-exec-ST-13d3ddad9198/bin/examples/dockerfile/buildx build ./examples/dockerfile --builder builder-docker ... (remaining 1 argument skipped)
error: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory
ERROR: listing workers for Build: failed to list workers: Unavailable: connection error: desc = "error reading server preface: EOF"
INFO: Elapsed time: 20.426s, Critical Path: 10.96s
INFO: 363 processes: 308 internal, 40 linux-sandbox, 15 local.
ERROR: Build did NOT complete successfully

@alexeagle alexeagle removed this from the 2.0 milestone Sep 6, 2024
@alexeagle alexeagle added the need: discussion Needs a proper discussion around the problem. label Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need: discussion Needs a proper discussion around the problem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants