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

Support setting appendix on Dockerfiles too #1144

Closed
wants to merge 3 commits into from

Conversation

yuvipanda
Copy link
Collaborator

Until now, 'appendix' was ignored for repos built with
their own Dockerfile. This fixes that, and sets the appendix
on repos built with Dockerfiles too.

I want to set a REPO_URL env var for all images built with
repo2docker-action
(jupyterhub/repo2docker-action#86)
and this is needed for that.

@yuvipanda
Copy link
Collaborator Author

https://github.com/jupyterhub/binderhub/blob/e04a8c3cec71ae8a1bf984da8623ecf8d81398bb/binderhub/binderspawner_mixin.py#L102 is how BINDER_REPO_URL is set. Instead, I just want to set REPO_URL in repo2docker itself.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding made explicit:

  • repo2docker can be run with an --appendix flag, but when a Dockerfile was created, it wasn't honored.
  • REPO_URL is the proposed name of a new environment variable to solve a problem.

It is my understand that a discussion about an env called REPO_URL would be a new environment variable. If that is correct, I'd very much like that REPO_URL instead is named REPO2DOCKER_REPO_URL or similar. Environment variables with a prefix related to the source code that defines them is a major help to avoid confusion, not only the the project that defines them but in other projects where the confusion otherwise propagates. I'm thinking for example on the amount of questions jupyterhub/z2jh/binderhub could get about REPO_URL that really was about repo2docker if we would leave out a REPO2DOCKER prefix.

tests/dockerfile/simple/verify Outdated Show resolved Hide resolved
repo2docker/buildpacks/docker.py Outdated Show resolved Hide resolved
repo2docker/buildpacks/docker.py Outdated Show resolved Hide resolved
@yuvipanda
Copy link
Collaborator Author

@consideRatio ah sure re: naming of the env var. I'll open a separate PR to deal with that, this PR doesn't touch that.

Until now, 'appendix' was ignored for repos built with
their own Dockerfile. This fixes that, and sets the appendix
on repos built with Dockerfiles too.

I want to set a REPO_URL env var for all images built with
repo2docker-action
(jupyterhub/repo2docker-action#86)
and this is needed for that.
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

@consideRatio
Copy link
Member

I labelled this as a bug because it wasn't documented that --appendix would only work for non-Dockerfile situations.

@yuvipanda yuvipanda requested a review from minrk March 15, 2022 08:13
@manics
Copy link
Member

manics commented Mar 15, 2022

Note this was discussed in a previous PR #1097 (comment)

@betatim
Copy link
Member

betatim commented Mar 15, 2022

I still like the logic/argument that manics linked to. This means "no appendix for customer Dockerfile based builds is a feature, not a bug".

@consideRatio
Copy link
Member

Ah hmmm...

I haven't formed a clear opinion if it is a feature or a bug, but right now it is an undocumented exception as I understand it - which i consider a documentation bug at least.

For now I'll leave a blank vote with regards to making it supported or explicitly fail/emit warning, and a strong vote for any of those options is better than undocumented and silently ignored situation we currently have. Ping me for further input, I feel I'd need to onboard myself a bit further to be able to think clearly what I consider to make sense about supporting it or not.

@manics
Copy link
Member

manics commented Mar 16, 2022

I've made an alternative proposal in jupyterhub/repo2docker-action#86 (comment)

@yuvipanda
Copy link
Collaborator Author

@manics I like that :)

@yuvipanda
Copy link
Collaborator Author

Going to close this as this was considered and rejected. I do agree with @consideRatio this should be documented though.

@yuvipanda yuvipanda closed this Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants