Skip to content

Feat: Add an onbuild install script step#600

Open
valentin-genin wants to merge 2 commits into
pangeo-data:masterfrom
CNES:pr/onbuild-install-script
Open

Feat: Add an onbuild install script step#600
valentin-genin wants to merge 2 commits into
pangeo-data:masterfrom
CNES:pr/onbuild-install-script

Conversation

@valentin-genin

Copy link
Copy Markdown

Add an onbuild step to install application (ex. novnc). Also add a clean up script after installation.
Necessary for #597 and #598 to work.

@github-actions

Copy link
Copy Markdown
Contributor

Binder 👈 Try on Mybinder.org!

This was referenced Jan 29, 2025
@valentin-genin valentin-genin marked this pull request as ready for review February 4, 2025 14:50
@valentin-genin

Copy link
Copy Markdown
Author

Hello @scottyhq, is it possible for you to review this PR if you have time ? If not, do you know someone I could ask for this ?

@scottyhq

Copy link
Copy Markdown
Member

@valentin-genin can you describe how you are using these images and why you need these proposed changes here versus on a fork? Now that the pangeo jupyterhubs are no longer functional unfortunately I think this repository will be getting less attention.

@guillaumeeb

Copy link
Copy Markdown
Member

Hey @scottyhq, let me chime in and try to explain!

@valentin-genin is working as a contractor at CNES. We have a Jupyterhub there, and already some Virtual Research Environments (VRE), which are basically pre-built docker and Singularity images. This images are built internaly and rely on some in-house components. Problem is: they are not really open or portable to other infrastructures/services like Pangeo like Jupyterhubs.

We thus decided to change a bit our way of doing things: propose really open source developement environments based on pangeo-docker-images (see https://github.com/CNES/datalabs-docker-images), and add our specific computing platform layer afterwards.

We think that some of our changes or addings might benefit a broader community than CNES collaborators, so we wanted to make them available on these pangeo-docker-images. I still think this repo and default images are really interesting to use on Binder like infrastructures. What's your opinion?

@scottyhq

Copy link
Copy Markdown
Member

I still think this repo and default images are really interesting to use on Binder like infrastructures. What's your opinion?

Thanks for the context @guillaumeeb. I really like and support your vision of easily moving between compute platforms. That said, looking briefly at this PR and the linked PRs I'm concerned that adding an arbitrary number of install.sh scripts run as root goes against the original design goals of compatibility with standard repo2docker configuration files and small size / fast build (https://pangeo-docker-images.readthedocs.io/en/latest/topic/design.html#design-goals).

The linked PRs that this change enables add a lot of additional dependencies to all images, so I feel that changes like this are better suited to a fork where you are free to do whatever you want (perhaps just modifying the base image so that you don't have to repeat the on-build steps for every image you're offering). It would be easy to add links to those CNES images in the docs so that others interested in more full featured customized images have a nice example to follow.

Comment thread base-image/Dockerfile
${NB_PYTHON_PREFIX}/bin/pip install --no-cache -r requirements.txt \
; fi

ONBUILD USER root

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is switching back to root necessary? In other words do you need the Python environment installed to successfully run install.sh?

Comment thread base-image/Dockerfile
; cp resources/layer-cleanup.sh /usr/local/bin/ \
; chmod +x /usr/local/bin/layer-cleanup.sh

ONBUILD RUN echo "Executing install scripts..." \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change would require documentation on how such a script is to be written and where it must live

@guillaumeeb

Copy link
Copy Markdown
Member

Thanks @scottyhq for the warm answer and suggestions! I have to admit that I did not looked into the PRs here, I was just aware of the functionalities added, and thought that this would be super helpful to have a VSCode extension and a remote desktop...

I'm currently looking a bit more into it, and have comments also. About this one:

  • Totally agree on your comments or questions. This needs at least documentation.
  • I'm a bit concern about the duplications of script in all images. Really needed? Could this be avoided?
  • Is the layer-cleanup script really important? This sounds like another feature, enabled by this one. Does it improve images sizes?
  • There is some licensing and copyright in the files.

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.

3 participants