Allow passing arbitrary extra arguments to repo2docker #96

yuvipanda · 2023-01-07T07:12:16Z

This adds an 'escape hatch', allowing the repo user to pass in additional arguments to
repo2docker that we may not explicitly have options for.
Adds another option to force a specific version of repo2docker to be installed. Potentially
helpful for reproducibiliy, but also helpful when we want to test an unreleased version of
repo2docker
Install some base python packages required to build repo2docker from source - this is
required if we want to install an unreleased version of repo2docker. This mostly brings in
packages present in the build stage of the upstream repo2docker image that we use:
https://github.com/jupyterhub/repo2docker/blob/main/Dockerfile

You can see an example of this in action in volcanocyber/VICTOR-notebook#6,
where we switch the base image to Ubuntu 22.04 by bringing in this PR: jupyterhub/repo2docker#909

Tests this repo2docker action PR: jupyterhub/repo2docker-action#96 to test this repo2docker PR: jupyterhub/repo2docker#909

GeorgianaElena · 2023-01-17T09:31:41Z

action.yml

+ description: Force a specific version of repo2docker to be installed. Either in the form of `repo2docker==<version>` for install from pypi, or `git+https://<github-url>@<commit-hash` for installing from a github repo / branch
+ require: false
+ EXTRA_PIP_INSTALL:
+ description: Extra packages to install in the image before running repo


Do we want this to be a required flag?
Update: this looks like it isn't actually used, so it could be removed?

Suggested change

description: Extra packages to install in the image before running repo

description: Extra packages to install in the image before running repo

require: false

I figure "in the image" is confusing as "the image" could be the GitHub action container image or the built repo2docker image. So I suggest "in the environment where repo2docker is run" instead.

But also since I suggested removing FORCE_REPO2DOCKER_VERSION in favor of sticking with EXTRA_PIP_INSTALL I figure we also need a note about using this to pin repo2docker's version.

And finally, this includes @GeorgianaElena require: false addition.

Suggested change

description: Extra packages to install in the image before running repo

description: Extra packages to install in the environment that runs repo2docker. You can for example use this to pin a specific version of repo2docker by providing `repo2docker==<version>` or `git+https://<github-url>@<commit-hash` to reference a repo2docker fork not on PyPI.

require: false

GeorgianaElena

Thanks @yuvipanda!

consideRatio

There are lengthy notes as it took a while for me to think this through, but I think I got this reduced to quite clear suggested changes making these review notes more of a background to my suggested changes.

Review notes

I figure ideally this should be three PRs as this adds three separate things, currently the title reflects only Add REPO2DOCKER_EXTRA_ARGS input I think. I care to a large extent because I want the changelog to be easy to follow from the changelog titles.

REPO2DOCKER_EXTRA_ARGS
The implementation of this is straight forward and seems ok, I can go for a quick merge on a PR adding this especially since a changelog entry is self explanatory with a title focused on adding that.
FORCE_REPO2DOCKER_VERSION
- I added an inline comment about why we added base-build python3-dev at all time to the custom Dockerfile we have.
EXTRA_PIP_INSTALL
- This is an input added to the action.yml but not implemented in any way. I suspect its a remnant after needing python3-dev to be able to build repo2docker.

Discussion points

Regarding FORCE_REPO2DOCKER_VERSION

Naming:
- I think the FORCE_ prefix is more verbose than we need to be
- I suspect _VERSION is a surprise as it really is a pip install entry of some kind.
Implementation:
- I suspect adding build-base and python3-dev setup time will increase quite notably even if this feature isn't used and don't feel great about it, what could be done? Depends, keep reading...
- If a GitHub Dockerfile based action that first builds an action and then runs it can be adjusted already while it builds the Dockerfile, then maybe we can make this be conditionally installed. I think though you can't. The documentation don't mention it and the Job's start out by preparing Dockerfile actions by building the Dockerfile, and when they do, no information is passed with build-args etc.

With all of this considered...

Suggested changes

Let REPO2DOCKER_EXTRA_ARGS be a dedicated PR very easy to merge quickly and include the name in the title
Transform this PR to a EXTRA_PIP_INSTALL + base-build and python3-dev PR where FORCE_REPO2DOCKER_VERSION is dropped. Let the PR title reflect that EXTRA_PIP_INSTALL is added. Document the input with a note that EXTRA_PIP_INSTALL can be used to install a specific repo2docker version.

consideRatio · 2023-01-17T09:24:20Z

Dockerfile

@@ -1,17 +1,9 @@
 FROM quay.io/jupyterhub/repo2docker:main

+RUN apk add --no-cache curl build-base python3 python3-dev py3-pip


Suggested change

RUN apk add --no-cache curl build-base python3 python3-dev py3-pip

RUN apk add --no-cache \

curl \

# build-base etc is installed to support a specified installation of

# repo2docker from source, these packages are the added difference

# between the build stage and the production stage in repo2docker's

# official Dockerfile:

# https://github.com/jupyterhub/repo2docker/blob/2022.10.0/Dockerfile#L5

build-base python3-dev py3-pip

consideRatio · 2023-01-17T10:21:09Z

@GeorgianaElena what do you think about my suggested changes above?

yuvipanda · 2023-01-18T04:51:02Z

@consideRatio I've split out just the extra-args into #97, will come back to the rest here once that is merged

I suspect the input variable is available only at runtime, not build time

hamelsmu · 2023-04-24T14:00:02Z

Is this good to merge?

yuvipanda · 2023-04-24T14:05:23Z

@hamelsmu yep!

hamelsmu · 2023-04-24T14:11:44Z

Ok merging. I would like the features in this PR as well 🙇🏽 Thank you for doing this.

P.S. @yuvipanda we should catch up at JupyterCon if you are going to be there

yuvipanda added a commit to yuvipanda/VICTOR-notebook that referenced this pull request Jan 7, 2023

Try switching to Ubuntu 22.04

e251453

Tests this repo2docker action PR: jupyterhub/repo2docker-action#96 to test this repo2docker PR: jupyterhub/repo2docker#909

yuvipanda mentioned this pull request Jan 7, 2023

Try switching to Ubuntu 22.04 volcanocyber/VICTOR-notebook#6

Merged

yuvipanda changed the title ~~Extra args~~ Allow passing arbitrary extra arguments to repo2docker Jan 7, 2023

yuvipanda marked this pull request as ready for review January 7, 2023 08:45

yuvipanda requested review from hamelsmu, consideRatio and GeorgianaElena January 7, 2023 10:46

GeorgianaElena reviewed Jan 17, 2023

View reviewed changes

GeorgianaElena approved these changes Jan 17, 2023

View reviewed changes

consideRatio requested changes Jan 17, 2023

View reviewed changes

consideRatio added the new label Jan 18, 2023

yuvipanda added 12 commits January 18, 2023 11:16

Add support for installing a specific version of repo2docker

a14156e

Try force it to work with bash

6d760d3

Fix shell conditionals

5a1ca60

Move pip install step into shell script

7cfbe1f

I suspect the input variable is available only at runtime, not build time

Add a debug statement

2e3a67a

Add build-base so ruamel.yaml will build

cb331b5

Install more python base packages, and cleaner

334c978

Install setuptools too

880127e

Invoke pip via python

975fea7

Install wheel and setuptools from pip instead

84abf1d

Upgrade setuptools maybe?!

1c3d530

Document FORCE_REPO2DOCKER_VERSION

9089a4f

yuvipanda force-pushed the extra-args branch from 232b347 to 9089a4f Compare January 18, 2023 19:16

Merge branch 'master' into extra-args

3fbf137

hamelsmu merged commit f0ac144 into jupyterhub:master Apr 24, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing arbitrary extra arguments to repo2docker #96

Allow passing arbitrary extra arguments to repo2docker #96

yuvipanda commented Jan 7, 2023 •

edited

Loading

GeorgianaElena Jan 17, 2023 •

edited

Loading

consideRatio Jan 17, 2023

GeorgianaElena left a comment

consideRatio left a comment •

edited

Loading

consideRatio Jan 17, 2023

consideRatio commented Jan 17, 2023

yuvipanda commented Jan 18, 2023

hamelsmu commented Apr 24, 2023

yuvipanda commented Apr 24, 2023

hamelsmu commented Apr 24, 2023

	description: Extra packages to install in the image before running repo
	description: Extra packages to install in the image before running repo
	require: false

		@@ -1,17 +1,9 @@
		FROM quay.io/jupyterhub/repo2docker:main

		RUN apk add --no-cache curl build-base python3 python3-dev py3-pip

-RUN apk add --no-cache curl build-base python3 python3-dev py3-pip
+RUN apk add --no-cache \
+ curl \
+ # build-base etc is installed to support a specified installation of
+ # repo2docker from source, these packages are the added difference
+ # between the build stage and the production stage in repo2docker's
+ # official Dockerfile:
+ # https://github.com/jupyterhub/repo2docker/blob/2022.10.0/Dockerfile#L5
+ build-base python3-dev py3-pip

Allow passing arbitrary extra arguments to repo2docker #96

Allow passing arbitrary extra arguments to repo2docker #96

Conversation

yuvipanda commented Jan 7, 2023 • edited Loading

GeorgianaElena Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

consideRatio Jan 17, 2023

Choose a reason for hiding this comment

GeorgianaElena left a comment

Choose a reason for hiding this comment

consideRatio left a comment • edited Loading

Choose a reason for hiding this comment

Review notes

Discussion points

Suggested changes

consideRatio Jan 17, 2023

Choose a reason for hiding this comment

consideRatio commented Jan 17, 2023

yuvipanda commented Jan 18, 2023

hamelsmu commented Apr 24, 2023

yuvipanda commented Apr 24, 2023

hamelsmu commented Apr 24, 2023

yuvipanda commented Jan 7, 2023 •

edited

Loading

GeorgianaElena Jan 17, 2023 •

edited

Loading

consideRatio left a comment •

edited

Loading