Gitlab integration development #92
Conversation
| @@ -59,7 +59,12 @@ def remotes_for_url(remote_url) | |||
|
|
|||
| # Find or create the correct remote for a fork with a given owner name. | |||
| def remote_for_request(request) | |||
| repo_owner = request.head.repo.owner.login | |||
| repo_owner = case request | |||
| when Request | |||
There was a problem hiding this comment.
What's the difference herey? Either request is a Request or what exactly could it be? I guess a comment might help here.
There was a problem hiding this comment.
Just fixed that, i explained in the commit message:
Back in the time, the request variable in this method could have been
something else when the Github provider didn't use the Request model.
Now this is always the case, so this check is unnecessary. In addition
the repo.head.user.login is cleaner to use than
request.head.repo.owner.login because the repo owner might get nil.
* create MR, list MR * close does not work * comments not implemented yet This is a very ugly first version, without tests
(rebase on development branch)
Back in the time, the request variable in this method could have been something else when the Github provider didn't use the Request model. Now this is always the case, so this check is unnecessary. In addition the repo.head.user.login is cleaner to use than request.head.repo.owner.login because the repo owner might get nil.
simply call request()
|
Hi @b4mboo i just rebased on latest development branch. I have been using this with my team for some time and it seems to work pretty well, would you consider reviewing this and let me know what we could improve or change? |
| @@ -57,7 +57,7 @@ def remotes_for_url(remote_url) | |||
|
|
|||
| # Find or create the correct remote for a fork with a given owner name. | |||
| def remote_for_request(request) | |||
| repo_owner = request.head.repo.owner.login | |||
| repo_owner = request.head.user.login | |||
There was a problem hiding this comment.
Is this really the same? Assuming the repository has multiple contributors, wouldn't it be possible that the latest commit has been made by someone else than the repo owner? In that case we'd end up with the wrong URL for the request.
There was a problem hiding this comment.
actually i reverted this but there is an issue here:
owner.login is not part of the model, it works with github because you pass a github object to the owner field that happen to respond to the login method.
There was a problem hiding this comment.
Oh, good catch. I've been working quite a bit on getting rid of all generic things fromgithub.rb. Thereby finding a couple of similar issues. I'm gonna put this on my list and try to add it to the model as soon as possible.
|
@brugidou Thanks for your contribution! I'm looking forward to extending git-review to Gitlab. Please take a look at my comments and tell me what you think. |
* use request.head.repo.owner.login * fix requests/request API
This follows #91
Please review the following changes:
This was quickly written to work with less features than the github version (namely: comments)
No tests yet so you don't have to merge this if you don't feel like it
It works with any gitlab server (when the string 'gitlab' is in the hostname) using Gitlab API v3
It uses the gitlab gem