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

Add repo2docker Dockerfile labels #500

Merged
merged 9 commits into from Dec 14, 2018

Conversation

@jrbourbeau
Copy link
Contributor

jrbourbeau commented Dec 13, 2018

This PR adds repo2docker version, repo URL, and ref labels to the generated Dockerfile

Closes #466

@jrbourbeau

This comment has been minimized.

Copy link
Contributor Author

jrbourbeau commented Dec 13, 2018

Just following up on #466 (comment), should repo be considered private and not included as additional metadata?

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Dec 13, 2018

Thanks for this nice PR!

I think repo shouldn't be private. Or put differently, if we do want to consider it private we'd also have to remove it from the information in .git and other places where it might leak.

One thing we are currently trying to improve for repo2docker is the time taken to execute the tests. This is making me think: can we avoid having to build the image and instead only generate the Dockerfile commands and/or inspect the return value of the function that injects the labels? Maybe a simple unittest here together with a check in an already existing test that builds an image (maybe in external/?) that the labels got set correctly would work? What do you think?

CHANGES.rst Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
repo2docker/buildpacks/base.py Outdated Show resolved Hide resolved
tests/test_labels.py Outdated Show resolved Hide resolved
@minrk

This comment has been minimized.

Copy link
Member

minrk commented Dec 13, 2018

re: private repo, it might be appropriate to use a single local string when building from a local directory instead of the path, which leaks info about the build machine's directory structure. I think it should be included if it's a real repo url, though.

@jrbourbeau

This comment has been minimized.

Copy link
Contributor Author

jrbourbeau commented Dec 13, 2018

can we avoid having to build the image and instead only generate the Dockerfile commands and/or inspect the return value of the function that injects the labels?

@betatim I think this would be a nice improvement! I noticed the picked BuildPack for a Repo2Docker instance only appears in the Repo2Docker.start() method. I could be missing something, but I didn't see a way to retrieve the picked_buildpack instance to use for testing. Would it be reasonable to add a something like

self._picked_buildpack = picked_buildpack

to the Repo2Docker class? That way we would have a reference to the corresponding chosen BuildPack which could be useful for testing purposes (and potentially other purposes too).

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Dec 13, 2018

I'm not quite sure yet how to do it, below a first draft/thinking out loud:

We call BuildPack.render() here. This outputs the Dockerfile that will be used to build the image. The render() call itself uses jinja2 to create the file. Can we use a mock to check that the jinja template was called with the right arguments (aka the labels are passed to the jinja template render call correctly? Then we don't have to call start_container() and can add --no-build to the argvs we set.

Overall we'd do something along the lines of repo2docker --debug --no-build https://example.com/here/repo | grep "LABEL correct-label-here" but smarter and more automatic :)

@jrbourbeau

This comment has been minimized.

Copy link
Contributor Author

jrbourbeau commented Dec 13, 2018

Adding a _picked_buildpack attribute removes the need to call start_container() because we can inspect the buildpack labels directly via something like app._picked_buildpack.labels. However, adding extra state to the Repo2Docker class, like a _picked_buildpack attribute, may be something we want to avoid, I'm not sure.

I pushed some commits that add a _picked_buildpack attribute and update the tests accordingly (note that images are not built in the updated tests). I can totally revert back adding _picked_buildpack if we want to go another direction, just thought it would be useful to have a clear example of what I was proposing.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Dec 13, 2018

I'm not a huge fan of adding the attribute just for testing purposes. What do you think of the approach with a mock?

The other way to keep the attribute is if it is useful for refactoring these methods (that are far too big) which @yuvipanda is working on.

@jrbourbeau

This comment has been minimized.

Copy link
Contributor Author

jrbourbeau commented Dec 13, 2018

I'm not a huge fan of adding the attribute just for testing purposes

Agreed. After updating the test to use a Mock approach, I think it's a cleaner solution.

@betatim betatim merged commit 7bcfa06 into jupyter:master Dec 14, 2018
4 checks passed
4 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 20%)
Details
codecov/project 75.23% (+0.12%) compared to eb27287
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Dec 14, 2018

Congratulations on getting your first contribution to repo2docker merged (and with such a cool PR number 🤓).

Thanks for the nice PR content and interaction!

@jrbourbeau jrbourbeau deleted the jrbourbeau:add-labels branch Dec 14, 2018
@jrbourbeau

This comment has been minimized.

Copy link
Contributor Author

jrbourbeau commented Dec 14, 2018

Thanks for the feedback and reviewing @betatim!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.