search

guyzmo / git-repo

Git-Repo: CLI utility to manage git services from your workspace
https://webchat.freenode.net/?channels=#git-repo
Other
842 stars 85 forks source link

Fix GitLab merge request fetch #138

Closed jayvdb closed 7 years ago

jayvdb commented 7 years ago

Reverts one chunk of e58373e2

jayvdb commented 7 years ago

c.f. https://docs.gitlab.com/ce/user/project/merge_requests/index.html for the fact that it should be merge-requests

guyzmo commented 7 years ago

errrrr… you're most likely right, but I did not invent those cassette records, so it has worked at some point. Let me double check the whys and hows,and test your fix.

But as the RTFM say so, I'm pretty sure it's alright and merge material 😉

jayvdb commented 7 years ago

I've asked if the branch name has changed at https://gitter.im/gitlabhq/gitlabhq?at=58c6bca109e7ba8510bcd274

guyzmo commented 7 years ago

@jayvdb you're welcome on irc.freenode.org #git-repo for a chat!

jayvdb commented 7 years ago

Can we please get some traction on this.

Please look at https://gitlab.com/gitlab-org/gitlab-ce/commit/86bccb71a1d4 , which is August 2015, ensuring that all MRs (even MRs from branches instead of forks) have a usable ref. The documentation there uses merge-requests. So for as long as it has been possible to always use a ref, the documentation has always said the ref will be merge-requests/*/head. (see moves like https://gitlab.com/gitlab-org/gitlab-ce/commit/7973a22f and https://gitlab.com/gitlab-org/gitlab-ce/commit/4f5bb980 to follow that original .md to its current location).

tests/integration/cassettes/test_gitlab_test_19_request_fetch.json and tests/integration/cassettes/test_gitlab_test_19_request_fetch__bad_request.json both have only one commit (e58373e21, 11 Oct 2016), and neither mentions merge-requests or merge_requests. ergo, the cassettes themselves have no opinion on whether it should be merge-requests or merge_requests.

That same commit had test_19_request_fetch using remote_branch='merge_requests' and the gitlab adapter changed:

-                       'merge-requests/{}/head'.format(request),
+                       'merge_requests/{}/head'.format(request),

But somehow you created those cassettes. My guess is that you obtained the cassette before the change, and then you changed the code above, thus creating the bug, and committed the cassettes and code changes together, resulting in a buggy test method. Anyway, it really doesnt matter if merge_requests ever worked somehow ... if it did, it was luck alone, as it isnt in accordance with the documentation.

guyzmo commented 7 years ago

Sorry for the loooong delay merging this small commit! But better late than never! 😉

I'm preparing a release for this week end, so stay tuned