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

Deploy app from git repository #445

Merged
merged 32 commits into from
Sep 16, 2024
Merged

Deploy app from git repository #445

merged 32 commits into from
Sep 16, 2024

Conversation

aktech
Copy link
Member

@aktech aktech commented Aug 9, 2024

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.

  • Add gitpython and conda-project as dependencies
  • Add /app-config-from-git/ endpoint to help frontend populate the deploy app form
  • Update spawner to use repository argument to deploy from repo
  • Add utility to get jhub-apps session to be able to call API in tests with authenticated cookies
  • Add unit tests
  • Add integration tests using the example repo: https://github.com/nebari-dev/jhub-apps-from-git-repo-example

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Documentation

Access-centered content checklist

Text styling

  • The content is written with plain language (where relevant).
  • If there are headers, they use the proper header tags (with only one level-one header: H1 or # in markdown).
  • All links describe where they link to (for example, check the Nebari website).
  • This content adheres to the Nebari style guides.

Non-text content

  • All content is represented as text (for example, images need alt text, and videos need captions or descriptive transcripts).
  • If there are emojis, there are not more than three in a row.
  • Don't use flashing GIFs or videos.
  • If the content were to be read as plain text, it still makes sense, and no information is missing.

Any other comments?

Copy link

vercel bot commented Aug 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
jhub-apps ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 16, 2024 7:30pm
jhub-apps-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 16, 2024 7:30pm

Copy link
Member

@krassowski krassowski left a 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 on jhsingle-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

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

jhub_apps/service/models.py Outdated Show resolved Hide resolved
jhub_apps/service/models.py Outdated Show resolved Hide resolved
jhub_apps/service/app_from_git.py Show resolved Hide resolved
jhub_apps/service/models.py Show resolved Hide resolved
jhub_apps/spawner/spawner_creation.py Outdated Show resolved Hide resolved
jhub_apps/service/models.py Outdated Show resolved Hide resolved
jhub_apps/spawner/spawner_creation.py Outdated Show resolved Hide resolved
@aktech
Copy link
Member Author

aktech commented Sep 16, 2024

Thanks a lot for the thorough review @krassowski

the cloned repository is deleted after the configuration is fetched

Yes, correct.

this uses jhsingle-native-proxy implementation for cloning thus we have less control over it and get more reliant on jhsingle-native-proxy which is not actively developed as of lately

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 jhsingle-native-proxy in future, but when? we don't know yet, depends on priorities.

it means that the spawn time will increase, especially for larger repositories

Yep, the best we can do is to clone with depth=1, which we already do.

it may make it harder to support private repositories as we would need to pass the PAT down to the spawner too

It's kind of supported via: https://<your_token>@github.com/username/repo.git (not that it's a priority in the PR)

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).

That makes sense.

Implicit assumptions as of this PR:

1 repository = 1 app

Not necessarily, as config_directory argument can be specified for various apps, like example:

  • app_foo (config_directory: foo)
  • app_bar (config_directory: bar)
  • app_car (config_directory: car)

repositories are public

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

repositories are small enough to not timeout during spawn and to not timeout when fetching the config

Yes, correct

jhub_apps/service/routes.py Outdated Show resolved Hide resolved
jhub_apps/service/app_from_git.py Show resolved Hide resolved
jhub_apps/service/app_from_git.py Outdated Show resolved Hide resolved
Copy link
Member

@krassowski krassowski left a 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 <[email protected]>
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

Successfully merging this pull request may close these issues.

2 participants