-
Notifications
You must be signed in to change notification settings - Fork 502
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
Introduce Gitlab support #524
Introduce Gitlab support #524
Conversation
Note that this is a work in progress, it's not ready to be merged. There are a bunch of tests that I want to add, I just want to discuss things a little bit before doing so to avoid having to rewrite them. |
@daddykotex I would rename this PR to Great work btw 👍 I'm going to give this a spin this week |
I didn't had the time yet to take a close look at all changes but the first thing I noticed is that working with GitLab or GitHub is configured at start-up of a scala-steward instance. Have you considered making this choice on a per-repo basis so that one instance can work with GitHub- and GitLab-hosted repos at the same time? For people like me who run a public instance it would be easier to support both this way. I'm not suggesting that you change your PR, but would like to hear your opinion if this would be feasible and if it would be small or big change compared to this PR. |
I did not think about it but it's a good suggestion. From the top of my head, the problem that would occur would be: you need different configuration for the API (user/pass/baseurl) when you are targeting different VCS. The most obvious solution to me is to transform the {
"github-public": {
"user": "scala-steward",
"pass": "/home/user/.github/askpass.sh",
"url": "https://api.github.com",
"repos": [
"tpolecat/skunk",
"travisbrown/catbird",
"travisbrown/iteratee",
"travisbrown/iteratee-twitter",
"twilio/guardrail",
"typelevel/cats",
"typelevel/cats-collections",
"typelevel/ca"
]
},
"gitlab-public": {
"user": "scala-steward",
"pass": "/home/user/.gitlab/askpass.sh",
"url": "https://gitlab.com/api/v4",
"repos": [
"other/random-repo"
]
}
} But I think it's content for another PR 😄 |
Agreed! Just wanted to hear your opinion, so thanks for that. :-) |
modules/core/src/main/scala/org/scalasteward/core/vcs/VCSApiAlg.scala
Outdated
Show resolved
Hide resolved
f168b36
to
0237cea
Compare
Also, with Gitlab I'm used to take comments into account by updating my commits. Gitlab UI is good enough to figure this out and show things correctly. I'm not sure if GitHub does the same but if that's a problem let me know, I'll just add commits on top of the other instead of rewriting. |
0237cea
to
caa3853
Compare
Since I don't have many comments on the changes I propose, I'll add more tests. I did not want to do it in the first place to avoid rewriting that. |
@daddykotex I just merged master into the |
This refactoring is breaking for users: * args change from --github-api-host to --vcs-api-host * args change from --github-login to --vcs-login
caa3853
to
f72e56a
Compare
Codecov Report
@@ Coverage Diff @@
## topic/gitlab #524 +/- ##
================================================
- Coverage 60.11% 51.66% -8.45%
================================================
Files 67 71 +4
Lines 875 871 -4
Branches 23 43 +20
================================================
- Hits 526 450 -76
- Misses 349 421 +72
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## topic/gitlab #524 +/- ##
================================================
- Coverage 60.11% 51.66% -8.45%
================================================
Files 67 71 +4
Lines 875 871 -4
Branches 23 43 +20
================================================
- Hits 526 450 -76
- Misses 349 421 +72
Continue to review full report at Codecov.
|
This is a work in progress. There are a lot of things to discuss: * the custom encoder/decoder to convert between GitHub json responses and Gitlab json responses * the Monad/MonadThrowabled requirements added because some operations in Gitlab are multi-step where as their original Github conterpart are a single step * the specific vcs thingy
f72e56a
to
83e9755
Compare
Thanks |
That's awesome! I'll test it with our internal GitLab very soon. |
Note that the changes in this PR are not yet on master. |
I just checked out the |
I've opened #645 to track remaining issues before GitLab support can be merged into master. |
Hey, Thanks for the feedback. I've got a few questions:
Thanks, I must admit that I did not retry it after the last rebase, maybe I did a mistake. |
To help you (so you don't have to look for it), here are the arguments I use when I run this on our internal GitLab installation:
|
Note that GitLab support has now landed in master (via #645). Many thanks again to @daddykotex! |
I just tried this with our GitLab server at work and it works like a charm! This is so cool, @daddykotex! :-) |
The problem GitHub and Gilab solves is similar (if not the same). So
most of the things apply to both. But somethings are different.
In GitHub, requests for changes are called Pull Requests (PR). In Gitlab,
they are called Merge Requests (MR). The rest of this PR will use both
in alternance. I'm used to Gitlab because of work so I use MR a lot :P.
This is a minor difference, in one case, GitHub uses Basic authorization
with the username/password. In the other case, GitLab uses a private token.
As I explained in this post,
the token can be generated from your GitLab settings page.
The token needs API permission.
The GitHub API exposes a list pull requests endpoint where you can explicitly
filter by owner and branch name with the
head
andbase
parameter. In GitLab,there is
source_branch
andauthor_id
. Right now this PR sets the source branchonly. I think it needs to be addressed. The problem is that we have the username
(string) but the
author_id
has to be an integer.See more here: https://docs.gitlab.com/ee/api/merge_requests.html#list-merge-requests.
I have two solutions in mind:
The first approach is easy but also error prone (provide an id that does
not match the token...).
The second one requires an additional request for some operations but would
look like that:
On GitHub, to create a pull request you post on the upstream project. For example,
if you were to create a pull request on scala-steward, you'd do:
The body contains the
base
which is the branch in which the changeswill be merged and the
head
which is where the changes are implemented.An example would be:
base => master
andhead => scala-steward:update/sbt-site-1.4.0
On Gitlab, it's different. The request has o target the fork instead of
the upstream repository. So if you'd like to open a similar merge request on
Gitlab, your request would look like that:
Where the body contains
target_project_id
,source_branch
,target_branch
.The
target_project_id
is an integer, and just like the user, we don't have it,so we have to fetch it first via an extra request. The source_branch and target_branch
have explicit names. An example would be:
target_project_id => 123
,source_branch => update/sbt-site-1.4.0
andtarget_branch => master
.Creating a fork is similar but there are a few differences. On GitHub,
you just POST on the upstream repository
/forks
endpoint. On Gitlab,you post to a similar endpoint but you provide a body. The body contains
an id and a namespace, like so:
The id the is the
owner/repo
url encoded and the namespace is the username.It also appeared (from my testing), the scala-steward tries to create fork
everytime it runs, and GitHub is just kind enough to say 202 Accepted even if
the thing is already existing. On Gitlab, you are rejected with a 409 Conflict.
This is why I added a MonadThrowable constraint so that I could catch the error
and recover from it. An alternative solution would be to check it before hand.
It's a minor difference, but on GitHub you have
open
andclosed
whereas Gitlab has
opened
andclosed
.There are probably things I went over and this pull requests is probably
not 100% working but I ran it a few times and I can say that: