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

Update Dockerfile to JH 1.0 #1291

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

vilhelmen
Copy link
Contributor

@vilhelmen vilhelmen commented May 20, 2019

Took me awhile to debug this one. Dockerfile doesn't actually point to JH 1.0 in #1263 @minrk

Perhaps it'd be better to remove the arg from the ci system and rely on the dockerfile to keep this from happening again?

@clkao
Copy link
Contributor

clkao commented May 23, 2019

I think it's better to also remove the buildArgs from chartpress.yaml.

@minrk
Copy link
Member

minrk commented May 24, 2019

I think I'd do the opposite - It wasn't updated in the Dockerfile because this value is never used, it is always set as the build arg by chartpress. So maybe we should have an invalid version in the arg to make it a non-optional arg: ARG JUPYTERHUB_VERSION=set-by-chartpress?

@vilhelmen
Copy link
Contributor Author

vilhelmen commented May 24, 2019

I feel like that would make it more difficult for those not using chartpress to generate images, myself included.

The way I see it, the dockerfile should be the complete configuration while the chartpress data should be scaffolding to assist in building the containers. I feel like making the dockerfile invalid by default would be a bad move.

Thinking back on other parts of our CI system, there are other bits that have generated-by-chartpress on occasion that has to be manually fixed, I think some helm chart data? I really feel that this broken-by-default behavior is a bad idea, though I recognize that it spreads version number configuration all over the place.

@minrk
Copy link
Member

minrk commented Jun 13, 2019

I feel like that would make it more difficult for those not using chartpress to generate images, myself included.

I don't think it's particularly helpful for this chart to support being built without the tools used to build this chart. One of the consequences is issues like this - needing to maintain unused configuration in multiple locations, which always results in the unused/unsupported paths being out-of-date. I think requiring chartpress is reasonable. Can I ask what's the impediment to building the images as intended with chartpress?

@vilhelmen
Copy link
Contributor Author

TBH I don't know much about chart press other than the name. It looks like less of a hassle than I thought, I'm not even sure how aware I was that it was a JH product. I agree with the versioning nightmare concerns. You're right. It's probably best to break the JUPYTERHUB_VERSION arg in the dockerfile to help deter others from cobbling together their own builds like I have been. I suppose I'll need to take an afternoon and rewrite our build system sometime soon.

@manics
Copy link
Member

manics commented Jun 14, 2019

Could defaulting JUPYTERHUB_VERSION to empty, and changing

$(bash -c 'if [[ $JUPYTERHUB_VERSION == "git"* ]]; then \
echo ${JUPYTERHUB_VERSION}; \
else \
echo jupyterhub==${JUPYTERHUB_VERSION}; \
fi')
to include
if [ -z "$JUPYTERHUB_VERSION" ]; echo jupyterhub be an acceptable compromise?

@vilhelmen
Copy link
Contributor Author

I like it, but it could be dangerous. I don't think it's a good idea. If JH releases a new version before we support it, it could cause issues. Or if you rollback to an old tag/branch, it will try to use the latest version.

@consideRatio
Copy link
Member

As @vilhelmen has use of this, and it is obviously better than having it say 0.9, I'll go ahead and merge this.

I'd support a change where we set it to something invalid to enforce the user to realize a good practice is to use chartpress (#1291 (comment)), and not mislead the user that this value has meaning for the deployment process which I thought.

@consideRatio consideRatio merged commit 445a953 into jupyterhub:master Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants