-
Notifications
You must be signed in to change notification settings - Fork 9
Add: Autostart #132
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
base: main
Are you sure you want to change the base?
Add: Autostart #132
Conversation
|
@GeraldIr this looks good to me! Do you mind writing up a couple sentences on what this solves and exactly what the expected behaviour here would be? And then am happy to test. It would also be nice to figure out writing a test for this - I can help with that once we have this outlined a bit. Thank you for your contribution! |
|
I've found the workflow of Build Image -> Start Image a bit cumbersome, so I was just looking for a way to simplify it down to a single press of a button. Ultimately I'd like to add an auto-start option for permalinks as well, in order to go from Click Permalink -> Running JupyterLab in a single click from anywhere without further user-input. This is a feature for the original BinderHub UI so that's what I'm trying to replicate. To be honest, I mistakenly opened the PR early, so for now I'll change this to a draft. |
|
@GeraldIr haha no worries - and yes, automatically starting the server once the image builds sounds really good to me and happy to help how I can to get this merged. For auto-starting on the Permalinks, let me open a separate ticket as well. In principle, this seems really nice - I think my only concern is if there's any security considerations to worry about - i.e. user clicks link and starts a server with a malicious image, etc. - the current way at least there is a bit of a confirmation step. Not sure how much of a concern this is as something we should worry about. Thanks again for jumping into this! |
|
I've included auto-start (build and start) from permalinks now. To trigger this, include the "autoStart":"true" in the Permalink values. I'll still need to include some tests, as well as hiding the Start button when the Build and Start Button is visible for the ImageBuilder (just confusing to have the vestigial button remain). |
|
As for the security concerns, as the standalone binderhub UI already works the same way I don't see much of an issue with this; but I am not a cybersecurity expert and usually take such concerns too lightly, so there is that 😅 |
|
I've now consolidated the Build and Start buttons from the custom image builder into a single button, as well as made it easier to create auto-starting permalinks, by including the autoStart option set to false by default when copying a permalink. |
|
@rtmiz 🙌 thank for you for this! I'll spend some time testing this tomorrow, but I will need to find some help to do a proper review as this does touch a fair bit of things. Could you add a short note, either to this PR or somewhere in the docs / code (I need to do a proper overhaul of the docs soon, so you can drop it here and I can use the text when I do the overhaul is fine) that:
I'll give this a whirl this week and let you know any feedback around usability, and then try and find someone who is able to do a proper frontend review - but in general, this is looking good to me, ty! |
|
cc @wildintellect @maxrjones - getting y'all's eyes on this: I think I might have seen you ask for something similar - would be good to get a 👍 from y'all on the general idea behind the feature and validating the usefulness would help me prioritize getting this merged. |
|
Of course, here's my rationale: I wanted to simplify the user experience down from 2 separate button clicks to 1. Previously the custom image building UI featured a "Build"-button, which built and pushed the image and then a "Start"-button, which initiated the spawning process of the previously built image. I couldn't think of a real use-case in which starting didn't follow building every time, so I merged the buttons. As for the auto-start feature, this is something we were looking for in our deployment and have now used to great success. We combined the autoStart option with multiple ?next= referrals (for login as well as invoking nbgitpuller) to cut down the workflow for users. This basically allowed us to generate URLs which are a 1-click referral to a specific environment with a specific notebook and it's been a great boon for tutorial sessions and our notebook gallery. Copied permalinks also now include a "autoStart": "false" by default, to make generating autoStart URLs easier. As for testing: Both features should be rather straightforward to test behavior on.
|
That sounds super-cool! If you have anything public that you're able to share, I'd love to get a better sense of how y'all are using this. |
|
@batpad You can find our notebook gallery here: https://eopf-sample-service.github.io/eopf-sample-notebooks/gallery/ From there select a notebook and click the Launch in JupyterHub button! You'll need to create an account though to use it, but otherwise it should be accessible. Also, there seems to still be some issues related to permalinks (values don't get updated if you opened the page via one and autoStart doesn't work yet for the build image page). I'm working on these right now. I can also share a piece of code when these issues are fixed that generates these URLs, this would also be of interest to @maxrjones for his second issue #137 |
|
To fix the issue I added a timeout when auto-starting so the form can properly load (this is a hack imo, but I don't know enough about react to fix this in a proper manner). @maxrjones Here's the code I mentioned earlier, make sure the image you are building will have nbgitpuller installed: |
|
I'd love for 2i2c to celebrate this work in a blog post. I invite @rtmiz and/or @batpad to share some quick information using this issue template. I thank my colleague @choldgraf for helping to make wins like this more visible to a wider audience. |
|
@yuvipanda The tests/linting now pass locally. |
|
@colliand I've posted the blog entry here: 2i2c-org/2i2c-org.github.io#527 |
|
Thank you @rtmiz ! |
|
@batpad No worries! |
| if (form) { | ||
| const button = form.querySelector("button[type=\"submit\"]") as HTMLButtonElement | null; | ||
| if (button) { | ||
| setTimeout(() => { |
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 seems like a neat hack indeed. Would just be curious to see if there's some-how a better way to do this, this does seem like it could get brittle and hard to debug in some edge case scenarios.
|
The code looks good to me and I think we should do a bit of testing and merge this in. @hanbyul-here would you be able to help with getting this over the line:
Looking at the code and thinking about this a bit, I think we can get this in before #122 and don't need to wait. Overall, this looks fine to me - in general, we need to fix the way Permalinks work, but that depends on upstream changes in Jupyterhub, so I do think we should go ahead with this and update docs to reflect this change. |
|
|
||
| renderWithContext(<ProfileForm />); | ||
| renderWithContext( | ||
| <form> |
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.
Needing to wrap the <form> around the <ProfileForm> in the tests makes sense to me - since fancy-profiles gets "injected" onto a page that already contains the container <form> tag, the <ProfileForm> expects to be rendered inside an existing <form> - since that does not exist during tests, we add it this way.
This looks a little awkward, but makes sense to me. Just giving @hanbyul-here some context while reviewing - not sure if there's a way to do this that feels more elegant, but this seems okay to me.
…false to copied permalink to make creating auto-start urls easier
…r full auto-start (login->profile-selection->notebook) configuration
|
@batpad I've rebased the changes from main and added the documentation for the feature to the existing myst pages. I've also included the code-snippet that I've posted above for nbgitpuller integration and handling login via URLs, I thought it might be a nice addition to complement this feature. |
Consolidation of Build and Start buttons in the custom image building UI into one button.
Permalinks now feature a autoStart field, that when set to true will instantly initiate the spawning process of the given environment.