Set labels when building image from Dockerfile

[TimoRoth]

We'd like to build our own custom images based on our own Docker Base-Images.
It works mostly fine, but the one big issue I ran into is that our Jupyter Hub does not show the images in its UI after successfully building them.

This is due to the labels it set on the command line not actually making it into the image. Adding them is easily achieved by adding this single line.

I also noticed that tljh makes use of an appendix to set some more stuff. So I extended this PR to have support for it as well.
For appendices to work, I needed to modify the Dockerfile on the fly, so I added that to the render method of the Docker BuildPack.
While making the build method based on that, I eventually realized that I had effectively re-implemented the base classes build method, with next to no differences.
And sure enough, just removing the build method entirely works like a charm.

This in turn meant that the label change had to move to the base class as well, where it could work in the exact same way, while simplifying the embedded Dockerfile a bit.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[manics]

This works because we pass **kwargs straight through to the build client:

• repo2docker/repo2docker/engine.py

  Line 179 in 4da2e1e

  **kwargs,

• repo2docker/repo2docker/docker.py

  Line 76 in 4da2e1e

  **kwargs,

• repo2docker/repo2docker/docker.py

  Line 90 in 4da2e1e

  **kwargs,

Should we tighten this up and make it a requirement for the builder to support labels, so **kwargs is only used for additional args?

[TimoRoth]

Made the labels parameter a proper argument on the engine, as suggested.
Also extended the scope of the PR a bit, since I noticed that tljh also relies on --appendix to work, which was so far not the case for Dockerfile based builds.
This somehow ended up simplifying the code a whole bunch.

[minrk]

👍 to adding labels, but the appendix was removed from custom dockerfiles on purpose in #250. It seems to be pretty context-dependent whether adding an appendix is appropriate for custom dockerfiles. I do like that it's very easy to understand what repo2docker does with custom Dockerfiles - no modifications. What assumptions can an appendix safely make of an arbitrary Dockerfile? It's a lot easier to write an appendix if you know it will be extending a generated Dockerfile from the standard templates, rather than requiring that it work for every possible Dockerfile. At least for BinderHub, I suspect the right thing to do is no appendix, so maybe this should be an off-by-default flag that custom Dockerfiles can be modified?

[TimoRoth]

On 23.11.2021 16:41, Min RK wrote: 👍 to adding labels, but the appendix was removed from custom dockerfiles on purpose in #250 <#250>. It seems to be pretty context-dependent whether adding an appendix is appropriate for custom dockerfiles. I do like that it's very easy to understand what repo2docker does with custom Dockerfiles - no modifications. What assumptions can an appendix safely make of an arbitrary Dockerfile?

tljh uses it to add labels via cli. There does not seem to be another way to do that via CLI. So adding a way to add labels via CLI would also be fine in my book, but it would need changes in tljh, which right now relies purely on --appendix for that.

It's a lot easier to write an appendix if you know it will be extending a generated Dockerfile from the standard templates, rather than requiring that it work for every possible Dockerfile. At least for BinderHub, I suspect the right thing to do is /no/ appendix, so maybe this should be an off-by-default flag that custom Dockerfiles can be modified?

I don't really see a downside to supporting the appendix with custom dockerfiles. It adds more flexibility for sure, and it seems more unexpected to me for the argument to just not do anything than for it to do maybe fail if you used it with a specific Dockerfile.

[TimoRoth]

Changed this to expose the labels as CLI options instead, hope that's more acceptable.
Just needs tljh adjusted to actually use it.

(Test failure seems unrelated, some external http resource failing)

[minrk]

Great! I definitely think it's reasonable to expect the appendix to be applied, especially for single-calls to jupyter-repo2docker. I also think it's reasonable as a repo author to expect your Dockerfile to be fully respected. Because both make sense, I think applying the appendix to the Dockerfie buildpack according to an option makes sense to me (and also warning if it's defined and not applied).

[TimoRoth]

With a way to externally set labels, I can resolve our main problem.
So I'll see if I can come up with a clean implementation for the appendix situation and open a separate PR.

[minrk]

FWIW, this is a perfect application of str.partition:

key, _, value = l.partition("=")

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[minrk]

Thanks!