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

feat(webhook): Add GitLab webhook end-point. #1448

Merged
merged 1 commit into from Aug 6, 2015
Merged

feat(webhook): Add GitLab webhook end-point. #1448

merged 1 commit into from Aug 6, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jul 18, 2015

Add a dedicated GitLab webhook end-point for push events on a
GitLab-hosted project.

The GitLab webhook end-point must be separate from GitHub as the
contents of GitLab's POST message, resulting from a push event, has
different payload data than what is found in an equivalent GitHub POST.

Closes #1442

Add a dedicated GitLab webhook end-point for push events on a
GitLab-hosted project.

The GitLab webhook end-point must be separate from GitHub as the
contents of GitLab's POST message, resulting from a push event, has
different payload data than what is found in an equivalent GitHub POST.
"project_id": 15,
"repository": {
"name": "Diaspora",
"url": "git@github.com:rtfd/readthedocs.org.git",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a real POST request from a gitlab commit hook? I'm asking because it includes github.com URLs which seem a bit odd. It won't make any difference in the logic but it will be confusing for future developers who look at the tests and see github URLs in the tests for gitlab.

Copy link
Author

Choose a reason for hiding this comment

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

@gregmuellegger, this payload data is based on the POST example provided by GitLab.

The modifications introducing github were necessary to pass the tests, as the project lookup matches the git url against github.com/rtfd/readthedocs.org.

@ghost ghost mentioned this pull request Jul 20, 2015
@ericholscher
Copy link
Member

I don't have a gitlab account, but is there a good way for us to test this locally? Or have you run it locally and confirm that it works?

@agjohnson
Copy link
Contributor

I'm hesitant on mangling data to fit the existing test case here. I'd rather this test what we can expect to see from Gitlab more precisely.

@ghost
Copy link
Author

ghost commented Jul 27, 2015

@ericholscher I'm not sure I would call GitLab's developer documentation easy. Though it's available if you're interested in setting up GitLab locally.

I did end up testing this change internally. Using a custom web hook in GitLab I was able to setup a project that, on a code push to master or a branch, the proper Read the Docs version was re-built.

@agjohnson I agree. I would prefer to use the GitLab example verbatim, but I'm not familiar enough with Read the Docs' testing fixtures to know what I need to change to support my GitLab tests.

@ericholscher
Copy link
Member

Feeling like we should get this merged in, and we can test it more in prod. Gitlab is something lots of people have wanted, so it makes sense to have in the codebase. It's annoying their dev docs aren't the best. We should add documentation though, before this gets merged in, so folks know how to use it.

@ericholscher ericholscher added the Needed: documentation Documentation is required label Jul 31, 2015
@ericholscher
Copy link
Member

I'm guessing the conflict is on our massive import renaming, so that should be an easy fix.

@ghost
Copy link
Author

ghost commented Aug 5, 2015

@ericholscher do you mean add documentation to https://read-the-docs.readthedocs.org/en/latest/webhooks.html?

As for testing what is sent from GitLab, I agree that the tests data is mangled a little, but the structure is not. I just replaced a lot of gitlab URLs with github URLs. Actually, that may not be to far from the use case as GitLab is used heavily as an on-premise source code platform by companies, so the URL could be pretty much anything.

However, I am more than willing to update the mock data to me more inline with what gitlab.com sends, but I would like to, if it's alright, wait until we can refactor how we provide mock data to the database ( as it will also effect #1446 ).

@ericholscher ericholscher merged commit 9002ff1 into readthedocs:master Aug 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: documentation Documentation is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants