pwntester / octo.nvim

Edit and review GitHub issues and pull requests from the comfort of your favorite editor
MIT License
2.24k stars 124 forks source link

Initial gitlab MR support #530

Open Daniel1854 opened 3 months ago

Daniel1854 commented 3 months ago

Describe what this PR does / why we need it

This PR enables meaningful interaction with gitlab MRs.

But honestly, I am not really sure if octo really wants this PR, mostly because of two things:

1) Two backends are probably more than twice the trouble to maintain. Implementing new features gets more complex due to the abstraction layer, and the need to implement at two places. Testing new changes gets more intricate, and increases the barrier to contribution. Documentary can easily get more confusing.

2) Recently the gitlab.nvim plugin got a lot of traction and there is lots of great work happening there as well. I worked on this PR mostly, because I dont like having golang/go libs as a dependency for my editor + I enjoyed using octo beforehand + I would have had to write a lot of go code to add the missing functionality I wanted. As a result I dont mind having to wait a little longer for everything to load (due to the conversion layer of the data structs).

Even if there is no intention to merge, I think its still interesting enough to share since someone else might checkout this branch and see for him-/herself if the current state is as useful to him/her as to myself. Regardless, it was quite fun to understand a little more about this plugin and the gitlab API :)

Does this pull request fix one issue?

References #282 and #138.

Describe how you did it

TL;DR: Introduced an indirection to each gh cli call, which now redirects to the corresponding backend. The github code has been moved, while the new gitlab code usually does a few more requests to match the information github got. It aggregates and converts those responses into the relevant fields of the github graphql requests.

The indirection is a little different from https://github.com/pwntester/octo.nvim/pull/370, because I choose to let the backend return a function instead of calling the backend with the name of a function and an options table, which gets unpacked into function arguments. This improves the LSP experience a ton and lets one jump to each backend function, while also having some form of typechecking at this interface.

The gitlab backend uses the glab cli tool for the http requests. The actual commands (e.g. glab mr list) are currently mostly suited for a human interpreter, but json as an output format for more commands is currently worked on afaik.

Describe how to verify it

I mostly tested with a small sample repo at gitlab and github. I am pretty confident that I didnt destroy anything major for github by moving the code, but I did focus on the MR Code and stayed away from Issues and Repo.

To test the gitlab part, you need to setup the officially supported glab cli tool. I implemented an identical feature set for telescope and fzf-lua.

Octo pr list results in a list of all MRs. When selecting one, the timeline with all threads (submitted, pending, resolved) and major events gets displayed. You can (un-)resolve those threads. You can create new labels, as well as add and remove labels to the MR.

Gitlab has no public concept of an on-going review, so only Octo review start is implemented. You are able to add and respond with pending notes. Those can be bulk published and an approve/revokation can be given via Octo review submit. Notes can also be deleted.

Special notes for reviews

Pagination - marked with # 244

Pagination works the same as gh: Paginated responses are just concatenated response arrays, which are terrible to parse. BUT gh can output the response as json via --jq ., and so each page is split by a newline. These are utilised to aggregate the pages.

This feature is missing with glab, but to print a newline after each page is pretty trivial: https://gitlab.com/Daniel1854/cli/-/tree/fix_paginated_stdout It is unclear however, if upstream would want to merge such a fix since they will probably implement something similiar to --jq as well. When using the build output of this cli worktree, one can use the following branch to enable pagination: https://github.com/Daniel1854/octo.nvim/tree/glab_pagination

Not intended features # 233 # 234

I had no interest in Issues, Milestones as well as project_cards (marked with # 233). I was somewhat interested in Emojiis, but they all got a special name instead of just unicode, so I didnt do it for the time being. Some of the gitlab code is still rough around the edges, and could use some more attention (marked with # 234), e.g. multiline comments - I didnt bother with calculating the lineranges.

The biggest # 234 in my opinion stems from the API not connecting the diff of a thread easily to the actual thread. This port relies on having fetched the Repo beforehand and git being able to find the commit SHAs.

Gitlab backend

The gitlab graphql API misses a lot of relevant connections, especially the discussions/notes/timeline endpoints are very separated. Some stuff seems just not accessible at all via graphql. Also, graphql requires a global_id of the (MR/Note) object, which I injected into the telescope/fzf-lua entry. Thats why 95% of all requests are towards the REST API with a lot of extra requests just to assemble everything github just got. This slows down the experience substantially.

brendalf commented 1 month ago

Nice work, @Daniel1854 . I agree 100% with your reasoning. I closed my previous PR as I was unable to work on it during the past year, but I happy to help maintain the new backend with follow-up features if this one ends up in the main branch.

pwntester commented 1 month ago

Awesome job @Daniel1854! Im not a gitlab user myself and barely have time to maintain the GitHub part of Octo but if you and/or @brendalf are able to become the CODEOWNER for this indirection layer, Im happy to get it merged into Octo

Daniel1854 commented 1 month ago

Sorry for the late reaction. I was/am feeling conflicted with the decision since I quit the job, where I used gitlab, few months ago. As it currently stands, I wont use this code for the next few years, which made me write this PR conservatively. Maybe I should have written that explicitly from the start.

I wouldnt mind overseeing the actual indirection layer, but it is certainly not great if there is no maintainer actively using the gitlab code as well. There seems to be only a handful of people interested in this PR, so Im just sadly not feeling convinced that it would be a good idea

Conarius commented 2 weeks ago

Hey @Daniel1854,

if you will not work on it now then I would help out to get it done and maintain it (maybe with someone else too?). Just tell me what needs to be done and I will gladly help out.

Daniel1854 commented 1 week ago

Sure, I summarised the main TODOs in the initial PR description. Let me know, if there is something in particular you think has to implemented. Pagination stands out to me as a non-negotiable, but that requires some lobbying. My main worry is that I havent tested it in big and random enough Pull Requests, so the biggest contributing thing one could do would be to actually use and observe it :+1: