Support the use of a URL root path #609
Conversation
| @@ -11,6 +11,10 @@ var defaultConfig = { | |||
|
|
|||
| // The base URL ungit will be accessible from. | |||
| urlBase: "http://localhost", | |||
|
|
|||
| // The URL root path under which ungit will be accesible. | |||
| // Either blank or starting with a slash and no trailing slash | |||
There was a problem hiding this comment.
Is this constraint enforced anywhere in the code? Seems like a user could easily violate this constraint without realizing it.
There was a problem hiding this comment.
I looked for other config params and there does not seem to be any validation for those.
Is there a good place to validate the config?
There was a problem hiding this comment.
Perhaps we can use url.resolve()? Not sure if we need additional validation though...
There was a problem hiding this comment.
I'm just saying that, from a UX perspective, it would be nice if ungit automatically appended a slash to the start of the config variable if needed, and removed any trailing slash. Just one less thing for the user to worry about.
Alternately, if taking care of this constraint automatically would take too much development effort, that requirement for the formatting of that setting should definitely be documented somewhere other than just a comment in the source code.
There was a problem hiding this comment.
I'll look into adding a validation for the complete url: urlBase+port+rootPath
There was a problem hiding this comment.
I'm not sure if I have explained correctly, if we do something like url.resolve(urlBase + ':' + port, rootPath) at the later part of this file, we don't need validation for checking prefix, postfix slashes within rootPath or port as it is handled via url package.
What I mean by additional validation is the character encoding and non alphabet characters and such....
|
@codingtwinky yeah exactly, and that's awesome that if it works now! We should remove the holdback code in the gruntfile @geeklain this is looking awesome. only thing I'm missing is a clicktest for this as well (see how for instance the authentication test works), so that this doesn't break in the future |
|
@FredrikNoren OK I'll look closer at the clicktest |
|
@FredrikNoren I modified the generic clicktest to test different rootPaths. I also added a rootPath clean up routine in config.js to cleanup leading and trainling slashes. I think that would help the users. |
geeklain
force-pushed
the
master
branch
2 times, most recently
from
b857107
to
c88ec00
Compare
|
@geeklain Ok awesome, looks ready to merge. Can you update the PR to solve merge conflicts and I'll merge it? |
|
@FredrikNoren All clear - rebased and tests passed - ready to merge |
geeklain
force-pushed
the
master
branch
4 times, most recently
from
919649f
to
870afcf
Compare
|
oh man! Tests keep failing because at least one of them encounters a weird error... Transient errors. It's driving me nuts |
|
Right, I have been having some issues with it as well. I have fixed some of them in #626 but as with any intermittent issues, it is hard to verify. |
|
Hurray! Tests passed without a hiccup after re-rebasing. |
Support the use of a URL root path
|
@geeklain Nice! |
|
We're going to use ungit on a shared environment, so we set up ungit for a few users, each with an own instance, through a reverse proxy. Everythings seems to work fine, except the favorite repositories on the home screen. I'm not quite sure, if we have set it up wrong or made some bad configurations, but we've a small fix in the home.js, which helps with this problem. |
|
@sebastianmay You might want to file a new issue or submit a pull request for that. It'd be easy for a comment like that in an already-merged PR to simply go unnoticed. |
|
@sebastianmay Yes, I do believe this is a valid issue and your fix is needed. If you would like, please do send us a pull request if you don't mind. Thanks! |
|
Somehow, I'm not able create a pull request from my fork. |
|
Huh. Yeah, that's weird. I was going to just send the PR for you since you've already got the commit in your fork, but GitHub's throwing a 500 error right now: FredrikNoren:master...sebastianmay:patch-1 The relevant commit is here: sebastianmay@2962d72 |
It adds support to run ungit under a specific URL root path like http://host:port/root/path .
This fixes #313
On the server side, the basic idea is to add a top level middleware in express that rewrites URLs to remove the root path and then moves forward to other middlewares (that do not need any change).
The socket io version had to be bumped up to better support the root path starting with a slash (http://socket.io/docs/migrating-from-0-9/#configuration-differences)
On the client side, there was quite a few changes to support the concept of a root path.
All the tests are succeeding.