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

Add GitLab repo sync and webhook support #1870

Closed
wants to merge 19 commits into from

Conversation

saily
Copy link

@saily saily commented Dec 20, 2015

still work in progress, but want to discuss with core team within this pull.

Implementation plan

@ericholscher
Copy link
Member

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.

@agjohnson
Copy link
Contributor

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.

@agjohnson agjohnson added the PR: work in progress Pull request is not ready for full review label Dec 21, 2015
@ghost
Copy link

ghost commented Dec 25, 2015

Can this be added to the GitLab milestone please?

@agjohnson agjohnson added this to the Gitlab milestone Dec 27, 2015
@agjohnson agjohnson changed the title WIP: Add basic GitLab repo sync support Add GitLab repo sync and webhook support Dec 27, 2015
@saily
Copy link
Author

saily commented Dec 29, 2015

good news, pennersr/django-allauth#1231 was merged into master.

@agjohnson
Copy link
Contributor

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 django-allauth in #1850, as I had some changes required there as well. #1893 addresses pinning to the next release.

OAUTH_SOURCE_BITBUCKET = 'bitbucket'

OAUTH_SOURCE = (
(OAUTH_SOURCE_GITHUB, _('GitHub')),
(OAUTH_SOURCE_GITLAB, _('GitLab')),
Copy link
Author

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.

Copy link
Contributor

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

Copy link
Author

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

@saily
Copy link
Author

saily commented Jan 9, 2016

@ericholscher, @agjohnson, please review.

I'll have to dig through your test-setup to implement some tests for this.
I saw you're using some s3 stuff in github/bitbucket testcases, don't you mock your request calls?
There some pretty good libs out there, example HTTMock.

@agjohnson
Copy link
Contributor

@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.

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']
Copy link
Contributor

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.

@scphantm
Copy link

is there a good point where i can jump in on this and give a hand. This issue is holding up our RTD implementation.

@agjohnson
Copy link
Contributor

@saily @scphantm The above feedback, along with test cases are still required. I don't have too much extra bandwidth to focus on this, but more than happy to guide anyone willing to wrap this work up. Gitlab integration would be a great addition!

@agjohnson agjohnson added Needed: documentation Documentation is required Needed: tests Tests are required labels Feb 26, 2016
@agjohnson agjohnson mentioned this pull request Mar 18, 2016
@BuddhaOhneHals
Copy link

@saily I added some tests and documentation for the GitLab integration:
https://github.com/galileo-press/readthedocs.org/commits/sync-gitlab-repos

There's a PR: saily#1

@BuddhaOhneHals
Copy link

@saily @agjohnson I addressed some of the annotations here: saily#2

@agjohnson
Copy link
Contributor

@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 agjohnson removed Needed: documentation Documentation is required Needed: tests Tests are required labels Aug 18, 2016
@BuddhaOhneHals
Copy link

@agjohnson sounds great! If you need any help please let me know.

Copy link

@connorshea connorshea left a 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.

GitLab
------

If your project is hosted on GitLab, you can manually set the webhook on Gitlab and

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

Copy link
Author

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?

Copy link

@ghost ghost Oct 19, 2016

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

: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

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

"""
session = self.get_session()

# See: http://doc.gitlab.com/ce/api/projects.html#add-project-hook

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

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.

Copy link

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.

Copy link
Author

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.

Copy link
Contributor

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.

:type organization: RemoteOrganization
:rtype: RemoteRepository
"""
# See: http://doc.gitlab.com/ce/api/projects.html#projects

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

If your project is hosted on GitLab, you can manually set the webhook on Gitlab and
point it at ``https://readthedocs.org/gitlab``:
Copy link

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 ?

Copy link
Contributor

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),
Copy link
Contributor

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.

Copy link
Contributor

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.

------

If your project is hosted on GitLab, you can manually set the webhook on Gitlab and
point it at ``https://readthedocs.org/gitlab``:
Copy link
Contributor

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.

jessetan referenced this pull request Dec 12, 2016
This is old and misleading.
Folks should generally just be using the GitHub & BitBucket hooks.
@segilbert
Copy link

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!

@parmentelat
Copy link

I was interested in using this feature, that's why I started with asking a related question on stackoverflow:
http://stackoverflow.com/questions/43023051/creating-a-readthedocs-io-repo-in-sync-with-a-public-gitlab-repo

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 ?

@ghost
Copy link

ghost commented Mar 27, 2017

@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.

@parmentelat
Copy link

@destroyerofbuilds : awesome, works like a charm ! Thank You :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants