-
Notifications
You must be signed in to change notification settings - Fork 248
Run ungit inside the datalab container #1047
Conversation
@@ -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 && \ |
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.
We should also add ungit to the about page: https://github.com/googledatalab/datalab/blob/master/sources/web/datalab/static/datalab.txt
@@ -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) { |
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.
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:
- Change the name of this function to something like 'isLocalPort'
- 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.
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 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"> |
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.
One comment I heard from Nikhil: Should this button (and ungit) only be there when running on a VM?
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.
Why not keep a consistent experience for all three supported scenarios?
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.