-
Notifications
You must be signed in to change notification settings - Fork 12
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
Deploy app from git repository #445
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
The code looks great, just two three tiny suggestions. Now on the architectural level I have some thoughts I would like to discuss:
Currently:
- the cloned repository is deleted after the configuration is fetched
- a new clone is performed during spawn time:
- this uses
jhsingle-native-proxy
implementation for cloning thus we have less control over it and get more reliant onjhsingle-native-proxy
which is not actively developed as of lately - it means that the spawn time will increase, especially for larger repositories
- it may make it harder to support private repositories as we would need to pass the PAT down to the spawner too
- this uses
In jupyter-gallery we would already have the repository cloned; in a future PR we may want to consider exposing an endpoint that allows to create an app directly using the config provided in the body of a POST request. This is why I am suggesting to rename AppConfigFromGit
to AppConfigFromCondaProject
or AppConfigFromConfigFile
if the url
attribute is not needed (as then we could reuse the response model for the logic of that proposed future endpoint).
Implicit assumptions as of this PR:
- 1 repository = 1 app
- repositories are public
- repositories are small enough to not timeout during spawn and to not timeout when fetching the config
Thanks a lot for the thorough review @krassowski
Yes, correct.
Yes, correct. It's not actively maintained at the moment, but we rely on it heavily at the moment. The plan is we may either replace it with some other solution (maybe oauth2proxy) or fork and maintain
Yep, the best we can do is to clone with depth=1, which we already do.
It's kind of supported via: https://<your_token>@github.com/username/repo.git (not that it's a priority in the PR)
That makes sense.
Not necessarily, as
The support for private repository isn't priority in the PR but it can be supported via specifying the url like: https://<your_token>@github.com/username/repo.git
Yes, correct |
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 think this is good to merge and iterate on to unblock frontend work. Thank you @aktech!
I left some tiny stylistic suggestions, but neither is blocking.
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Reference Issues or PRs
Implements #422
This PR implements an endpoint named:
/app-config-from-git/
this will be used by frontend to fetch the app deployment details from the git repository. This would let the frontend pre-fill the app creation form. After this frontend will make the app deployment request on the same/server
endpoint with git repo's extra details.gitpython
andconda-project
as dependencies/app-config-from-git/
endpoint to help frontend populate the deploy app formrepository
argument to deploy from repoWhat does this implement/fix?
Put a
x
in the boxes that applyTesting
Documentation
Access-centered content checklist
Text styling
H1
or#
in markdown).Non-text content
Any other comments?