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.

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

Support URL parameters to populate Dynamic Image Building component #66

Open
batpad opened this issue Aug 27, 2024 · 5 comments
Open

Support URL parameters to populate Dynamic Image Building component #66

batpad opened this issue Aug 27, 2024 · 5 comments
Assignees

Comments

@batpad
Copy link
Collaborator

batpad commented Aug 27, 2024

When we enable dynamic image building on the hubs, we want users to be able to share URLs with an environment specified to be able to share. Hence, fancy profiles should support populating the dynamic image building fields based on passed in URL parameters.

We eventually want to support a similar / same URL format to what mybinder.org supports, so users would be able to go to a URL like https://staging.hub.openveda.cloud/service/fancy-profile/v2/gh/batpad/foo and be correctly redirected to the hub spawn page and have the dynamic image building component selected with the correct provider and repository based on gh/batpad/foo. This redirect would happen server-side and the user would land up on a URL that looks like https://staging.hub.openveda.cloud/hub/spawn?binderProvider=gh&binderRepo=batpad%2Ffoo&binderRef=main

With those query parameters, the fancy profiles code should pre-fill the dynamic image building fields with the correct values (and select the dynamic image building option).

So, on fancy profiles, I think the work involved is:

  • Make the form more like the form on mybinder.org: i.e. split up into three separate fields for provider, repository URL and ref / branch. Ideally, we would support all providers as mybinder.org but for now it is ok if we only support GitHub. The "repository" field should allow a user to paste in a full Github URL and handle parsing that out into the provider / repository.
  • Once we have this split up into three separate fields, handle reading the query parameters for binderProvider, binderRepo and binderRef to populate these fields.
  • Ensure the values get correctly submitted to the Binder backend to build the image correctly

The names binderProvider, binderRepo and binderRef are very provisional - @yuvipanda let know if you have a better idea for naming here - but also, this URL scheme should not be considered stable and is something we might change down the line.

Currently, we won't support handling the case where Profile Options have dynamic image building components within multiple profiles on the page. Currently, if there are multiple profiles with dynamic image building enabled, we can just show an error. In the next PI, we can think through handling selecting profiles in the URL as well, and then make this work for the case when there is a dynamic image building option within multiple profiles. For our current deployments, we will stick to having a single profile.

Refs NASA-IMPACT/veda-jupyterhub#45

cc @oliverroick

@oliverroick
Copy link
Collaborator

I don’t quite understand the rationale for the redirect and the use of url params in the redirect URL when we have all the information we need in the original URL.

We can unpack the gh/batpad/foo in https://staging.hub.openveda.cloud/service/fancy-profile/v2/gh/batpad/foo and populate the form with that information.

Is it possible to render the form under https://staging.hub.openveda.cloud/hub/spawn/gh/batpad/foo, ie. any path below /hub/spawn renders the form. The we can unpack the remainder of the URL in the front-end?


Make the form more like the form on mybinder.org: i.e. split up into three separate fields for provider, repository URL and ref / branch.

We’re currently submitting the form via HTTP POST, so the backend would have to be changed to account for the new form-field structure if we make this change.

I’d suggest to keep this simple though, and stick to two fields:

  1. The full URL of the repository
  2. The branch

@yuvipanda
Copy link
Member

We’re currently submitting the form via HTTP POST, so the backend would have to be changed to account for the new form-field structure if we make this change.

The backend only gets the built final image information via unlisted choice, it doesn't actually see the repo or ref we send out. So the frontend talks to the binderhub API, provides it with repo, ref and provider, and gets back the built image. This built image is what is actually submitted to the JupyterHub backend. The JupyterHub backend doesn't know anything about repo, ref, etc.

The reason for needing the provider be a field is that we support many different providers, not just git:

image

And some of them (like zenodo) do not accept refs, only the 'repo'.

Eventually we would need to auto detect the provider from what the user pastes - that's the rationale behind introducing 'detect' regexes in my rewrite (https://github.com/jupyterhub/binderhub/pull/1856/files#diff-b1d57a27bde93c65a3fdd528ffae65f4aabf048cdc545f59f90ea13d489a110cR711).

However, right now, I think it's ok to just ask for the repository and the ref (which can be branch, tag, HEAD, etc). And we only support the gh provider (github).

@yuvipanda
Copy link
Member

Is it possible to render the form under https://staging.hub.openveda.cloud/hub/spawn/gh/batpad/foo, ie. any path below /hub/spawn renders the form. The we can unpack the remainder of the URL in the front-end?

I had assumed this is currently not possible, hence the suggested query parameters. I just tested with https://hub.openveda.cloud/hub/spawn/test and I get a 404, so my assumption was right. We could have JupyterHub support this, and by stating that the current query params are not to be considered stable, we can eventually do that instead of using a separate service. I do agree this would be a better call perhaps than doing this as a service, but not sure what implications for jupyterhub itself would be.

@batpad
Copy link
Collaborator Author

batpad commented Aug 28, 2024

I don’t quite understand the rationale for the redirect and the use of url params in the redirect URL when we have all the information we need in the original URL.

We can unpack the gh/batpad/foo in https://staging.hub.openveda.cloud/service/fancy-profile/v2/gh/batpad/foo and populate the form with that information.

Is it possible to render the form under https://staging.hub.openveda.cloud/hub/spawn/gh/batpad/foo, ie. any path below /hub/spawn renders the form. The we can unpack the remainder of the URL in the front-end?

@oliverroick I hear you. I think the rationale for proposing the redirect for me is roughly:

Pros

  • As a practical matter, routing everything under /hub/spawn/ to the front-end app would require deeper changes in JupyterHub. This seems like it could get a bit tricky, especially as jupyterhub-fancy-profiles is still an optional configurable add-on to a JupyterHub instance. There exists a well defined interface in JupyterHub to add a backend service and handle a redirect, and doing it this way will not require changes in core JupyterHub routing code.
  • Personally, I think these options are more logical as query parameters rather than as part of the route - especially as we plan more parameters in the future to select other profile options. I think we'd end up with a mix of things been defined in the route and in query parameters and there not being a clear logic for why something is in the route versus query parameters. For me, it seems a bit easier to reason about from the JS side with all these options being query parameters. Happy to hear other thoughts.
  • The frontend does not need to care about the "binder compatibility" layer, and can design the interface as needed, and the backend service just provides that "redirect shim" or so to maintain URL-compatibility.
  • We need to create a backend service anyways because the way we are over-riding the default kubespawner template currently will be deprecated and the way to do it would be creating a separate service handler. In this case, it seems not too much over-head to add a route to do this redirect.

Cons

  • We have this additional piece of backend code handling the redirect and we will need to make sure this redirect code doesn't mess things up as we add more features, etc. It will mean we have to test our frontend code handling query parameters but then also that the redirecting and query parameter passing all works correctly when hitting the backend service handling the redirect. However, there is already quite a lot of fairly complex URL redirecting happening around the login flow, and so we would in general need to test redirects work properly anyway.

@oliverroick at the end, I think this idea was driven more by back-end architectural decisions - and to me, it seemed nice for the frontend to not have to worry about these URL compatibility details and can design a flexible query parameter structure.

And sorry, I know a lot of this thinking through happened in conversation between me and @yuvipanda - hope some of this context helps, and happy to talk through :)

We’re currently submitting the form via HTTP POST, so the backend would have to be changed to account for the new form-field structure if we make this change.

It's probably helpful to go through the exact flow for the "Dynamic Image Building" widget. Will be good to have @yuvipanda walk us through the details here on our call tomorrow. Roughly as I understand it: there's a separate call to the Binderhub API with the provider, repository and ref and the Binderhub API handles building the image and in the end, returning the URI for the built docker image. We then just include the docker image path (I think in a hidden field) in the final POST request for the form submission.

Hopefully that flow will be clear after we go through it together.

Thanks @oliverroick for the thoughtful comments - I realize that some of the details here are likely lost in 1:1 conversations between me and @yuvipanda so it's very helpful to have some of these things identified so we can clarify rationale / larger context and get more perspective on some of these choices.

@oliverroick
Copy link
Collaborator

@yuvipanda do we still need to keep this ticket open?

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

No branches or pull requests

3 participants