-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add GitLab repo sync and webhook support #1870
Conversation
Sounds like a good plan! Thanks for working on this. Let us know if you have questions or need guidance. I know @agjohnson has done some work on these areas recently, so he might have some thoughts as well. |
I was just talking about the ability to extend this to Gitlab, thanks @saily for digging into it. I'm not sure on the best way to handle this. I'm currently in the same code, have refactored some already, and really wish to refactor the rest of the existing code, as it's a mess. It really should be written in a more constructed pattern, and there are some missing relationships that should have been added. I'll try to get my work updated shortly, we might need to address a refactor in this PR before merging. |
Can this be added to the GitLab milestone please? |
good news, pennersr/django-allauth#1231 was merged into master. |
I've updated my work in #1850, and that PR is now in for review. We should get to it next week. That PR would be a good basis for this work, as the abstractions for OAuth services are structured cleanly there. I've already pinned |
7b37bc2
to
e6de58d
Compare
readthedocs/oauth/constants.py
Outdated
OAUTH_SOURCE_BITBUCKET = 'bitbucket' | ||
|
||
OAUTH_SOURCE = ( | ||
(OAUTH_SOURCE_GITHUB, _('GitHub')), | ||
(OAUTH_SOURCE_GITLAB, _('GitLab')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is required, @agjohnson can you please check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this isn't required, it should have been refactored out. Dropped in #1912
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i'll drop it and update my PR
@ericholscher, @agjohnson, please review. I'll have to dig through your test-setup to implement some tests for this. |
@saily the test cases at https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/rtd_tests/tests/test_oauth.py are only using fixture data responses, we're not mocking the requests. This might be a good addition though. We mock requests and responses without httpmock in other tests. |
readthedocs/oauth/services/gitlab.py
Outdated
repo.ssh_url = fields['ssh_url_to_repo'] | ||
repo.html_url = fields['web_url'] | ||
repo.private = not fields['public'] | ||
repo.clone_url = fields['http_url_to_repo'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clone_url
should be set to ssh_url
if this is a private repository.
is there a good point where i can jump in on this and give a hand. This issue is holding up our RTD implementation. |
@saily I added some tests and documentation for the GitLab integration: There's a PR: saily#1 |
Sync gitlab repos
@saily @agjohnson I addressed some of the annotations here: saily#2 |
@BuddhaOhneHals thanks for picking up the last leg of this PR! Because this PR is sizable, not very digestible, and has been active for such a long time, I think we should try testing this locally some as well. If things look good after a final review and some local testing, I think we can get this finally merged and deployed! |
@agjohnson sounds great! If you need any help please let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments for this PR. If we can help in any way with getting this pushed through, we'd be happy to.
docs/webhooks.rst
Outdated
GitLab | ||
------ | ||
|
||
If your project is hosted on GitLab, you can manually set the webhook on Gitlab and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be formatted as GitLab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitLab branding is to always call it GitLab.com
(though I can't find their public documentation on that at this time).
However it's usually referred to as GitLab
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
readthedocs/oauth/services/gitlab.py
Outdated
:param url: start url to get the data from. | ||
:param kwargs: optional parameters passed to .get() method | ||
|
||
See http://doc.gitlab.com/ce/api/README.html#pagination |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This URL should be updated to https://docs.gitlab.com/ce/api/README.html#pagination
readthedocs/oauth/services/gitlab.py
Outdated
""" | ||
session = self.get_session() | ||
|
||
# See: http://doc.gitlab.com/ce/api/projects.html#add-project-hook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for this one.
|
||
:param fields: dictionary of response data from API | ||
:param privacy: privacy level to support | ||
:param organization: remote organization to associate with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Organizations are called Groups in GitLab, not sure if that matters or if that'd be confusing for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Groups
would be better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@connorshea, @destroyerofbuilds of course you're both right, but i had to adapt the given api and this api uses organization
. My recommendation would be to get this into master and then discuss how to refactor the oauth-plugin system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
organization
in this case is our internal representation of the various providers names for orgs/groups/what have you. I don't think this usage needs to reflect GitLab's naming.
readthedocs/oauth/services/gitlab.py
Outdated
:type organization: RemoteOrganization | ||
:rtype: RemoteRepository | ||
""" | ||
# See: http://doc.gitlab.com/ce/api/projects.html#projects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this URL as well.
Support private repositories and cleanup
docs/webhooks.rst
Outdated
------ | ||
|
||
If your project is hosted on GitLab, you can manually set the webhook on Gitlab and | ||
point it at ``https://readthedocs.org/gitlab``: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this webhook be updated to reflect the new webhook end points added in #2433 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, without a webhook management page, these webhooks aren't completely deprecated yet. Once that feature exists, this page will need to be updated to instead reference that page and drop information on manual set up. Likewise, we should add a deprecation notice on GitHub Services for RTD as well. I'll open an issue outlining the work that will go into this one -- fine to point to this webhook endpoint for now.
'merge_requests_events': False, | ||
'note_events': False, | ||
'tag_push_events': True, | ||
'url': u'https://{0}/gitlab'.format(settings.PRODUCTION_DOMAIN), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #2433 these webhook endpoints were deprecated. A new handler for GitLab will be required and this should be updated. We can either address this in the PR, or I'm also +1 on filing a new PR to address this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, uh, I already implemented this.
Nothing to see here.
docs/webhooks.rst
Outdated
------ | ||
|
||
If your project is hosted on GitLab, you can manually set the webhook on Gitlab and | ||
point it at ``https://readthedocs.org/gitlab``: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, without a webhook management page, these webhooks aren't completely deprecated yet. Once that feature exists, this page will need to be updated to instead reference that page and drop information on manual set up. Likewise, we should add a deprecation notice on GitHub Services for RTD as well. I'll open an issue outlining the work that will go into this one -- fine to point to this webhook endpoint for now.
This is old and misleading. Folks should generally just be using the GitHub & BitBucket hooks.
My team is looking to leverage GitLab integration. When do you expect this merge request will be processed and released into the wild? Thanks for all this work. Looking forward to this feature! |
I was interested in using this feature, that's why I started with asking a related question on stackoverflow: Rephrasing the question here: in the meanwhile (until this PR makes it to deployment on gitlab.com), is there a way to achieve the same result (pushes on gitlab triggers a build on readthedocs) by manually adding a webhook on the gitlab side ? |
@parmentelat you can setup a webhook on GitLab to trigger a build on readthedocs for a project hosted on GitLab. The webhook already existed, but the documentation was never upstreamed. I'm contributing @takotuesday's documentation work in #2747. Please take a look at that documentation on how to get started with configuring your project with a webhook. |
@destroyerofbuilds : awesome, works like a charm ! Thank You :) |
Implementation plan
django-allauth
.gitlab
.