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

Docker build rule doesn't work at the top level #616

Closed
endobson opened this issue Nov 17, 2015 · 13 comments
Closed

Docker build rule doesn't work at the top level #616

endobson opened this issue Nov 17, 2015 · 13 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: bug

Comments

@endobson
Copy link
Contributor

If I have a docker build rule at the top level of a bazel workspace the generated script for loading it in to local docker repository doesn't work. This is because the tag generated for the image is rejected by docker because it has an empty name section.

BUILD rule

load("/tools/build_defs/docker/docker", "docker_build")

docker_build(
  name = "minimal_racket"
)
endobson@yggdrasil () ~/tmp/bazel-project % bazel run :minimal_racket                                                                                                                                                                  (0)
INFO: Found 1 target...
Target //:minimal_racket up-to-date:
  bazel-bin/minimal_racket-layer.tar
INFO: Elapsed time: 0.829s, Critical Path: 0.00s.

INFO: Running command line: bazel-bin/minimal_racket.
Skipping 8d1835c9303e14989c4e17d218c78419e321526abe4db5f772dffdf0876e5cd6, already loaded.
Skipping ac8cc867c44ca0a4b90edbbb3fb00d3f47baef63b83e74880da0d4c89de836c2, already loaded.
Tagging ac8cc867c44ca0a4b90edbbb3fb00d3f47baef63b83e74880da0d4c89de836c2 as bazel/:minimal_racket
repository name component must match "[a-z0-9](?:-*[a-z0-9])*(?:[._][a-z0-9](?:-*[a-z0-9])*)*"
ERROR: Non-zero return code '1' from command: Process exited with status 1.
@damienmg
Copy link
Contributor

workaround by specifying the tag: bazel run //:minimal-racket my.repository/minimal-racket

I am unsure of a correct solution for that.

@damienmg damienmg added type: bug P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) cloud / appengine / docker labels Nov 17, 2015
@damienmg damienmg modified the milestone: 0.5 Jun 14, 2016
@damienmg damienmg removed this from the 0.5 milestone Nov 14, 2016
@linuxerwang
Copy link
Contributor

How about adding a "custom_repository_name" attribute so user can specify arbitrary repository names? Not only for this scenario, but also for other use cases in which the generated repository names might contain more than two levels while the docker registry supports only two levels, such as: goharbor/harbor#1179.

If it's acceptable, I'd volunteer to implement it.

@linuxerwang
Copy link
Contributor

Here is a draft impl: linuxerwang@ebd5d66

@damienmg
Copy link
Contributor

@linuxerwang: Why not simply use the repository attribute?

@damienmg
Copy link
Contributor

Your draft impl is actually changing the image name, not the repository part.

@linuxerwang
Copy link
Contributor

linuxerwang commented Apr 11, 2017

You are right, Damien, I meant to make the image name customizable, not the repository.

Code updated: linuxerwang@320e725

Thanks!

@damienmg
Copy link
Contributor

Sorry for the delay, holidays... The code looks good, it would be accepted as a PR I think.

/cc @mattmoor: fyi there is still plenty of issue on docker_build on the bazel repo

@mattmoor
Copy link

Hmm, @damienmg I thought this was fixed a while ago, but I guess not.

For custom naming in general, folks should use docker_bundle. For the top-level naming issue, we should use the morale equivalent of os.path.join(ctx.attr.repository, ctx.label.package) instead of "%s/%s" % (ctx.attr.repository, ctx.label.package)

@linuxerwang
Copy link
Contributor

@mattmoor I didn't get how to use docker_bundle to customize naming, could you please provide an example? Also, it'd be quite cumbersome to define another target just to get the image name right, since we have lots of such places. I feel docker_bundle is design to help combining images rather than naming purpose.

The os.path.join() trick has been applied: #2807

Thanks.

@mattmoor
Copy link

Left a couple comments on the PR.

In principle, yes it is intended for bundling several images, to just alias it you'd write:

docker_bundle(
   name = "my_bundle",
   images = {"gcr.io/foo/bar/baz:blah": "//my/image:name"},
)

I wasn't suggesting aliasing would fix this issue, ultimately I think _join_path is the simplest/correct fix. My point was that if you want to alias the image for other reasons you can utilize this trick.

If you do in fact want aliasing throughout your codebase of docker_build rules, I'd be curious to learn a bit more about your use case (feel free to reach out to me via email). If it is just to avoid docker-tagging prior to push, then I'd also recommend taking a look at the recently release docker_push rules.

@linuxerwang
Copy link
Contributor

linuxerwang commented Apr 18, 2017

@mattmoor Thanks, I get it now.

The reason why we have to customize the image names a lot is because our folder structure is as follows:

WORKSPACE/
    |-- project
            |-- cmd
                  |-- command1
                         |-- BUILD

such that the generated image name would be: registry.com/project/cmd/command1 which has 3 levels of folders. But the docker registry we are using (https://github.com/vmware/harbor) only supports 2 levels.

Another reason is that historicly some of the binaries/images disperse to different projects, but we want to name them with common prefix, such as payment/service and payment/queue (just demo names). The "service" and "queue" are under different projects but we want both have the same prefix "payment".

Code updated. PTAL.

@mattmoor
Copy link

FWIW, I'd like to separate the discussion of aliasing the built images from a fix for this particular issue. I think we can close this issue by changing two lines to use _join_path and no signature changes.

I'll open another issue where we can discuss aliasing.

@linuxerwang
Copy link
Contributor

Code updated with only the two lines change. PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: bug
Projects
None yet
Development

No branches or pull requests

4 participants