Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial gitlab MR support #530

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

Daniel1854
Copy link
Contributor

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 #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 brendalf mentioned this pull request May 14, 2024
4 tasks
@brendalf
Copy link

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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 👍

Abstract calls away from the GH cli through an indirection.
Pick backend cli program based upon remote host link.

This seems like the most sensible approach since it maintains type hints
through luals.
Enables meaningful interaction with gitlab MRs.
telescope and fzf-lua are supported.

Gitlab has no public concept of an ongoing review,
so only `Octo review start` is implemented.

Displays all threads (submitted, pending, resolved).
Able to add and respond with pending notes.
Bulk publish all pendings notes and approve/revoke
via `Octo review submit`.
@legobeat
Copy link

legobeat commented Jul 9, 2024

GitLab support aside, the indirection layer is exciting if it means that adding future support for other forges like Gitea/Forgejo or Radicle becomes much easier than the great collective effort spent on this already. +1 for decoupling the higher layers from the GitHub API.

Aside One thought I had: Would an alternative be to go the other way by leaning all the way in on `gh` and instead look at making a `gh`-API-compatible CLI utility that Octo.nvim could call appropriately depending on remote?

Then one could for example make a translation layer external to Octo that behaves like gh but interacts with GitLab. Could be a gh-gitlab which would just pass through the same parameters with the right flags to gitlab-cli.

But seeing how GraphQL-heavy the current usage of the API is, not sure how valuable such an approach would be.

@rocket-engineer
Copy link

Hi @Conarius,

I can help you out with the integration and the maintenance. Since I use GitLab at work and for private projects as well (self-hosted GitLab CE), I'm looking forward to get this pushed to the master.

I've already played a little bit with the new GitLab backend, GitLab's REST API and GraphQL as well. But I have to mention that I'm quite new to octo.nvim ...

@Conarius
Copy link

So, after some initial testing I can say the following:

  1. The gh backend works just fine
  2. The glab backend has some issues

Many telescope/fzf-lua functions are missing like issues, search and repos.
Then when I use the list prs command it throws this error:

Error executing vim.schedule lua callback: ...scope.nvim/lua/telescope/finders/async_static_finder.lua:16: bad argument #1 to 'ipairs' (table expected, got nil)
stack traceback:
	[C]: in function 'ipairs'
	...scope.nvim/lua/telescope/finders/async_static_finder.lua:16: in function 'new_table'
	...y/octo.nvim/lua/octo/backend/glab/telescope-commands.lua:88: in function 'cb'
	.../share/nvim/lazy/octo.nvim/lua/octo/backend/glab/cli.lua:111: in function ''
	vim/_editor.lua: in function <vim/_editor.lua:0>

@Daniel1854
Copy link
Contributor Author

Daniel1854 commented Aug 21, 2024

Many telescope/fzf-lua functions are missing like issues, search and repos.

Yeah, I personally am not interested in those features, but feel free to add them if you are interested

Error executing vim.schedule lua callback: ...scope.nvim/lua/telescope/finders/async_static_finder.lua:16: bad argument #1 to 'ipairs' (table expected, got nil)

It seems like the request, that fetches all open MRs for that Repo, returns no valid MRs and thus no table is created. This could be due to no open MRs and bad handling of the response (but I think, the present is_blank handling should catch that..) or due to bad parsing. I pushed a minor improvement to the error handling, but I dont see how that case would be reachable in the first place.
Have you made sure that the glab cli is actually able to fetch PRs? In the case of Octo pr list, the relevant request will be glab api "/projects/:id/merge_requests?state=opened".

Btw, which version of glab and which branch are you using for the tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants