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.

Sign up for GitHub

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

Jump to bottom

Refactor image build check #56

Merged
merged 4 commits into from
Oct 12, 2018
Merged

Refactor image build check #56

merged 4 commits into from
Oct 12, 2018

Conversation

rokroskar
Copy link
Member

Move the logic regarding image selection out of the spawner and into the notebook service. The spawner should just launch whichever image is given to it.

@rokroskar rokroskar added the enhancement New feature or request label Sep 21, 2018
@rokroskar rokroskar added this to the 0.2.1 milestone Sep 21, 2018
@rokroskar rokroskar changed the title Refactor gitlab ci image build check [WIP] Refactor gitlab ci image build check Sep 21, 2018
@rokroskar rokroskar changed the title [WIP] Refactor gitlab ci image build check Refactor gitlab ci image build check Sep 21, 2018
@rokroskar
Copy link
Member Author

This seems to work, tested on staging. Would be good to have a second pair of eyes.

@rokroskar rokroskar changed the title Refactor gitlab ci image build check Refactor image build check and add RStudio launch Sep 24, 2018
@rokroskar rokroskar changed the title Refactor image build check and add RStudio launch Refactor image build check Sep 24, 2018
leafty
leafty requested changes Oct 1, 2018
View reviewed changes
Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Question: with this change, would it be possible for a user to launch a private image without having access to it?

helm-chart/renku-notebooks/values.yaml
@@ -89,8 +96,6 @@ jupyterhub:
extraEnv:
DEBUG: 1
JUPYTERHUB_SPAWNER_CLASS: spawners.RenkuKubeSpawner
## timeout for waiting on an image build
# GITLAB_IMAGE_BUILD_TIMEOUT: 600
Copy link
Member

Choose a reason for hiding this comment

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

Need to move it instead, no?

@rokroskar
Copy link
Member Author

@leafty I guess you might be right since we don't have user-level image registry secrets. I was more thinking that we may want to offer a selection of base images to launch from, hence the added query option.

@leafty
Copy link
Member

leafty commented Oct 1, 2018

I see. I guess it might be good to rethink the PR then. Let's see how we can have public image selection together with repo-based images.

@rokroskar
Copy link
Member Author

@leafty I am happy to remove that part for now as it doesn't really address the point of this PR in any case. We may want to look at the JupyterHub spawner options_form feature which allows you to do something along these lines and control which images are allowed.

jirikuncar
jirikuncar reviewed Oct 2, 2018
View reviewed changes
helm-chart/renku-notebooks/templates/deployment.yaml Outdated
@@ -48,6 +48,14 @@ spec:
value: {{ .Values.jupyterhub.hub.services.notebooks.oauth_client_id }}
- name: GITLAB_REGISTRY_SECRET
value: {{ template "renku.fullname" . }}-registry
- name: GITLAB_URL
{{ if .Values.gitlab_url }}
Copy link
Contributor

Choose a reason for hiding this comment

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

.Values.gitlab.url (see https://github.com/SwissDataScienceCenter/renku-notebooks/pull/56/files#diff-4a2cd40c069f23f7cd71ab6a74028709R10)?

@rokroskar
Copy link
Member Author

@leafty can you please review again?

leafty
leafty approved these changes Oct 12, 2018
View reviewed changes
helm-chart/renku-notebooks/templates/deployment.yaml
{{ if eq .Values.debug true }}
- name: FLASK_DEBUG
value: "1"
{{ end }}
- name: JUPYTERHUB_API_TOKEN
value: {{ .Values.jupyterhub_api_token }}
value: {{ .Values.jupyterhub.hub.services.notebooks.api_token }}
Copy link
Member

Choose a reason for hiding this comment

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

👍

singleuser/Dockerfile
@@ -1,12 +1,12 @@
# Build as renku/singleuser
# Run with the DockerSpawner in JupyterHub

ARG JUPYTERHUB_VERSION=0.9
ARG JUPYTERHUB_VERSION=0.9.2
Copy link
Member

Choose a reason for hiding this comment

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

they made 0.9.1 disappear, will this one last?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's there: https://github.com/jupyterhub/jupyterhub/releases/tag/0.9.1

@rokroskar rokroskar mentioned this pull request Oct 12, 2018
Merged
@rokroskar rokroskar merged commit 97b0d7e into master Oct 12, 2018
@leafty leafty mentioned this pull request Oct 16, 2018
Closed
@leafty leafty deleted the refactor branch October 19, 2018 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jirikuncar jirikuncar jirikuncar left review comments

@leafty leafty leafty approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rokroskar @leafty @jirikuncar