-
Notifications
You must be signed in to change notification settings - Fork 6
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
custom images for interactive environments #1109
Conversation
BREAKING CHANGE: Requires SwissDataScienceCenter/renku-notebooks#304 fix #1105
Now it verifies whether a default image has been used for an interactive environment, giving feedback to the user. It's not deployed yet, but it can be tested using the image built from this PR SwissDataScienceCenter/renku-notebooks#452 |
Preview added here: https://lorenzotest.dev.renku.ch/
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good, and the changes are easy to follow. I would like a different terminology for the "custom" images. I do not think that word is very descriptive. I suggest "pinned." I also think most users do not care which image is being used, so I suggest changing the new environment form to display the information about the pinned image like this:
The "more info/pinned image" link should take the user to documentation about this feature.
@rokroskar what do you think?
@@ -472,12 +482,16 @@ class NotebookServerRowCompact extends Component { | |||
} | |||
} | |||
|
|||
function getStatusObject(status) { | |||
function getStatusObject(status, defaulImage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaulImage
-> defaultIamge
globalWarning = ( | ||
<Warning key="globalWarning"> | ||
The project configuration for interactive environments | ||
contains {language.article}variable{language.plural} that {language.aux} either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I addressed the 2 points from the review, but I used slightly different words to avoid the disputed Redeploying to https://lorenzotest.dev.renku.ch is taking longer due to the docker pull rate limit. Running through telepresence works when paired with the latest version of renku-notebook currently on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the agreement on terminology in SwissDataScienceCenter/renku#1737, I think we can just use "pinned image."
<Input type="input" disabled={true} id="customImage" style={style} value={projectOptions.image}></Input> | ||
<FormText> | ||
<FontAwesomeIcon className="no-pointer" icon={faInfoCircle} /> This project specifies | ||
an <ExternalLink role="text" iconSup={true} iconAfter={true} url={url} title="image in the settings" />. A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that SwissDataScienceCenter/renku#1737 has been approved, I think we can go with
This project specifies a <ExternalLink role="text" iconSup={true} iconAfter={true} url={url} title="pinned image" />.
@@ -924,12 +975,16 @@ class StartNotebookPipelinesBadge extends Component { | |||
text = "not available"; | |||
} | |||
} | |||
else if (pipelineType === NotebooksHelper.pipelineTypes.customImage) { | |||
color = "primary"; | |||
text = "custom"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be changed to text = "pinned";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic! 🎉
Allows users to start interactive environments with a custom image.
A user can check the image used for an environment by clicking on the ready icon. Not sure if this is the best way to show this information (despite it shouldn't be very important). Any alternative suggestions are welcome.
Preview: https://lorenzotest.dev.renku.ch
P.S. Trying to start a new environment when an image is not available (both custom image or commit image) will result in getting a
404
response. The UI needs to show proper feedback in that case -- this is still missing.fix #1105