Add support for custom repo resolvers #27
Conversation
There was a problem hiding this comment.
I admit I don't quite understand why this is needed, but it sounds like you have a need for it, and this PR is a reasonable addition.
I've left some comments for how this can be improved.
Mostly it's low on documentation. There's one suggestion to move var err error so it has a smaller scope, since it's possible.
Otherwise this LGTM.
| } | ||
|
|
||
| // Resolve a URL path to a repo location | ||
| type PathResolver func(info ResolveInfo) (string, error) |
There was a problem hiding this comment.
Comment should begin with "PathResolver ...", see https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences.
| type ResolveInfo struct { | ||
| Path string | ||
| Request *http.Request | ||
| } |
There was a problem hiding this comment.
Are you using a struct here instead of making PathResolver accept those 2 parameters because you expect there may be changes, and you want to be able to do that without breaking the API?
There was a problem hiding this comment.
I was following the pattern that the auth info uses of having a struct, which allows for future extension without breaking the API. I was thinking that ideally it would be nice to have a context dictionary in the struct instead of passing request all the way through the layers, but wasn't obvious how to do that without a larger refactor since the auth is it's own independent middleware.
| @@ -23,6 +31,8 @@ type GitHttp struct { | |||
|
|
|||
| // Event handling functions | |||
| EventHandler func(ev Event) | |||
|
|
|||
| PathResolver PathResolver | |||
There was a problem hiding this comment.
golint would say:
warning: exported field PathResolver should have comment or be unexported
I think it's important to document that PathResolver is optional and can be nil. Otherwise, people who don't need to use this will be unsure what value to set here.
There was a problem hiding this comment.
I'll clean up the golint messages.
| func (g *GitHttp) getGitDir(file_path string) (string, error) { | ||
| func (g *GitHttp) getGitDir(file_path string, req *http.Request) (string, error) { | ||
|
|
||
| var err error |
There was a problem hiding this comment.
You can move var err error inside the if statement, it's not needed outside.
| Request *http.Request | ||
| } | ||
|
|
||
| // Resolve a URL path to a repo location |
There was a problem hiding this comment.
This documentation doesn't seem sufficient. As someone who reads this for the first time and tries to use it, I have the following unanswered questions:
- What is URL path? Is it the path component of a URL?
- If so, then why is it provided as a separate
Pathfield insideResolveInfo, is it different fromRequest.URL.Path? - What does repo location mean, is is a system filepath (e.g., "C:\MyRepo" on Windows and "/home/user/MyRepo" on Linux) where the repo is? Or something else?
It'd be helpful to document this better.
| @@ -10,6 +10,14 @@ import ( | |||
| "strings" | |||
| ) | |||
|
|
|||
| type ResolveInfo struct { | |||
| Path string | |||
| Request *http.Request | |||
There was a problem hiding this comment.
golint would warn that all 3 of these things are exported and should have comment or be unexported.
Is Path the same as Request.URL.Path? If so, are both needed?
There was a problem hiding this comment.
The repo path is extracted from a regex when it looks up the service, since the full request path might have /info/refs or other things after it, so the path of the repo and the path of the request will be different.
| @@ -17,6 +17,8 @@ type AuthInfo struct { | |||
| // But could also be: "some_repo.git" | |||
| Repo string | |||
|
|
|||
| Request *http.Request | |||
There was a problem hiding this comment.
golint would say:
warning: exported field Request should have comment or be unexported (golint)
jezell
force-pushed
the
custom-resolver
branch
from
ed12cab
to
9cc09d4
Compare
jezell
force-pushed
the
custom-resolver
branch
from
9cc09d4
to
f8aec15
Compare
|
Cleaned up the PR. Looks like there are a bunch of unrelated golint messages. I wouldn't mind cleaning those up in another PR, but it could break consumers since it is complaining about things like casing on existing structures. |
My use case for this functionality would be: I run go-git-http on multiple horizontally scaled servers for reliability. If a user tries to fetch a repository I could use a custom resolver function to check whether or not that repo exists, then if it doesn't, reach out to a central data store (S3, etc) to fetch the data and hydrate the repo on disk so that the git binary can serve it. |
| @@ -216,7 +231,16 @@ func sendFile(content_type string, hr HandlerReq) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (g *GitHttp) getGitDir(file_path string) (string, error) { | |||
| func (g *GitHttp) getGitDir(file_path string, req *http.Request) (string, error) { | |||
|
|
|||
There was a problem hiding this comment.
Nitpick. This blank line doesn't seem to be helpful. You can remove it.
We needed the ability to add custom resolver logic. Our resolvers also require information from the request such as the host from the request, and the resolve can be optimized to happen at the same time we check auth, so having access to the request in both auth and resolvers is very useful. This change does the following: