Skip to content
This repository was archived by the owner on Sep 3, 2022. It is now read-only.

Run ungit inside the datalab container #1047

Merged
merged 3 commits into from
Nov 14, 2016

Conversation

yebrahim
Copy link
Contributor

This PR adds ungit inside the datalab container. It runs on port 8083 inside the container, and gets its requests reverse proxied so only the one port used by datalab is open outside the container.

I added a git icon in the app bar to open ungit in the current location. It finds the current directory path and points ungit to it. Note that ungit will ignore any trailing subdirectories after the git root dir.

image
image

@@ -24,6 +24,9 @@ RUN ln -s /datalab/web/static/datalab.css /datalab/nbconvert/datalab.css && \
cd /datalab/web && /tools/node/bin/npm install && \
cd / && \

# Install ungit
/tools/node/bin/npm install -g ungit@0.10.3 && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -48,12 +48,22 @@ export function getRequestPort(request: http.ServerRequest, path: string): strin
}

/**
* Checks if a request should be exempted from reverse proxying to the gateway
*/
function shouldSkipGateway(port: String) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a long time to realize why this logic was doing what it was doing, and I think that a slightly different structure would have made it easier to understand the reasoning.

I'd suggest the following:

  1. Change the name of this function to something like 'isLocalPort'
  2. Reorder the conditional branches in the 'handleRequest' method. With this change, what was previously the else branch is conceptually the default branch, so it feels like it should come first in the order.

Alternatively, we could make the 'isLocalPort' method check the 'KG_URL' environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the naming isn't good. I think it's a better idea to make the function check the KG_URL though, but it should be checking if traffic should be proxied, not local. Please take a look and let me know what you think.

@@ -13,6 +13,11 @@
</span>
<div class="btn-toolbar pull-right">
<div class="btn-group">
<button id="ungitButton" title="Open ungit here" class="toolbar-btn">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment I heard from Nikhil: Should this button (and ungit) only be there when running on a VM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keep a consistent experience for all three supported scenarios?

@yebrahim yebrahim merged commit 0706f2e into googledatalab:master Nov 14, 2016
@yebrahim yebrahim deleted the yebrahim-ungit branch December 6, 2016 18:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants