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.

Sign up for GitHub

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

Allow to skip a build on commit messages #3457

Closed
wants to merge 24 commits into from
Closed

Allow to skip a build on commit messages #3457

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f6d9780
Change GitHub webhook
stsewd Dec 28, 2017
e851bf0
Change GitLab webhook
stsewd Dec 28, 2017
e21af7e
Change BitBucket webhook
stsewd Dec 28, 2017
634a6ed
Change spec for general webhook
stsewd Dec 28, 2017
3eea78b
Validate for commits containing a skip mark
stsewd Dec 28, 2017
177f0f7
Fix comments
stsewd Dec 29, 2017
f20fcd3
Fix wrong key
stsewd Dec 29, 2017
8150793
Update tests from test_api.py
stsewd Dec 29, 2017
1c89e6a
Update test_integrations.py
stsewd Dec 29, 2017
64e00b0
Better log message
stsewd Feb 18, 2018
435726c
Use requests for encode payload
stsewd Feb 18, 2018
24dd729
Merge branch 'master' into allow-skip-build
stsewd Feb 18, 2018
a930dc3
Check commit message no object
stsewd Feb 18, 2018
517d3eb
Update test payload
stsewd Feb 18, 2018
8ab67be
Make generic webhook backwards compatible
stsewd Feb 18, 2018
12147c8
Update payload
stsewd Feb 18, 2018
b808ce4
Set webhooks from api v1 compatible
stsewd Feb 18, 2018
c8b8756
Linter
stsewd Feb 18, 2018
836c9ca
Add tests
stsewd Feb 18, 2018
806789d
Update generic API parameters
stsewd Feb 18, 2018
97235a3
Add details to skip a build
stsewd Feb 18, 2018
5803557
Change list -> array
stsewd Feb 18, 2018
ce0fa0f
Merge branch 'master' into allow-skip-build
stsewd Apr 11, 2018
808f835
Fix tests
stsewd Apr 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion readthedocs/core/views/hooks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,23 @@ def _build_version(project, slug, already_built=()):
return None


def _contains_skip_mark(commit):
Copy link
Member

Choose a reason for hiding this comment

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

Receive the message itself, instead of relying on a commit object.

"""
Check if a commit message has a skip mark, for example: [skip docs],
[docs skip].
"""
skip_marks = [
r'\[skip docs?\]',
r'\[docs? skip\]',
]
message = commit['message']
for skip_mark in skip_marks:
mark = re.compile(skip_mark, re.IGNORECASE)
if mark.search(message):
return True
return False


def build_branches(project, branch_list):
"""
Build the branches for a specific project.
Expand All @@ -75,9 +92,17 @@ def build_branches(project, branch_list):
not_building - a list of branches that we won't build
"""
for branch in branch_list:
versions = project.versions_from_branch_name(branch)
commit = branch['last_commit']
versions = project.versions_from_branch_name(branch['name'])
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change branch by branch['name'] here?

Copy link
Member Author

Choose a reason for hiding this comment

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

See #3457 (comment)

to_build = set()
not_building = set()
if _contains_skip_mark(commit):
log.info('(Branch Build) Skip build %s', project.slug)
Copy link
Member

Choose a reason for hiding this comment

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

Add the reason of the skip, like "because contains skip mark" or something better :)

not_building = {
version.slug
for version in versions
}
return (to_build, not_building)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the logic for skipping the build, not sure if this is the correct place

Copy link
Member

Choose a reason for hiding this comment

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

So, if the last commit contains the mark we don't build any version?

I'm confused here since the next few lines there is a logic that can build some version but not some others comming in the same webhook, right? Shouldn't we apply a similar logic here also?

That to_build and not_building rely on the function _build_version. So, this logic shouldn't be in that method? (I don't know, but is has the logging integrated and returns what it needs. On the other hand, it seems that we shouldn't call that method if we already know that we need to skip the version but there are some logic to skip versions inside that function too :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm confused here since the next few lines there is a logic that can build some version but not some others comming in the same webhook, right? Shouldn't we apply a similar logic here also?

Of what I understand, a branch/tag/commit can be tied to various versions so there is only one commit message for all versions, this means that all versions from this branch aren't going to be built. And also is about what you mention, we already know if the version isn't going to be built.

for version in versions:
log.info("(Branch Build) Processing %s:%s",
project.slug, version.slug)
Expand Down
76 changes: 72 additions & 4 deletions readthedocs/restapi/views/integrations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,16 @@ class GitHubWebhookView(WebhookMixin, APIView):

{
"ref": "branch-name",
"head_commit": {
"id": "sha",
"message": "Update README.md",
...
}
...
}

See full payload here:
https://developer.github.com/v3/activity/events/types/#pushevent
Copy link
Member Author

Choose a reason for hiding this comment

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

Are we using the v3 of the gh api?

Copy link
Member

Choose a reason for hiding this comment

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

It seems, https://developer.github.com/v3/#current-version, since we are not sending the Accept header and v3 is the default.

Copy link
Member Author

@stsewd stsewd Feb 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

And also v4 is using GraphQL, this will break everything.

"""

integration_type = Integration.GITHUB_WEBHOOK
Expand All @@ -159,7 +167,12 @@ def handle_webhook(self):
# Handle push events and trigger builds
if event == GITHUB_PUSH:
try:
branches = [self.data['ref'].replace('refs/heads/', '')]
# GitHub only returns one branch.
branch = {
Copy link
Member Author

Choose a reason for hiding this comment

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

I change branch to be a dict with name and last_commit as elements.
last_commit must have an id and message.

'name': self.data['ref'].replace('refs/heads/', ''),
'last_commit': self.data['head_commit'],
}
branches = [branch]
return self.get_response_push(self.project, branches)
except KeyError:
raise ParseError('Parameter "ref" is required')
Expand All @@ -177,8 +190,16 @@ class GitLabWebhookView(WebhookMixin, APIView):
{
"object_kind": "push",
"ref": "branch-name",
"commits": [{
"id": "sha",
"message": "Update README.md",
...
}]
...
}

See full payload here:
https://docs.gitlab.com/ce/user/project/integrations/webhooks.html#push-events
"""

integration_type = Integration.GITLAB_WEBHOOK
Expand All @@ -191,7 +212,13 @@ def handle_webhook(self):
# Handle push events and trigger builds
if event == GITLAB_PUSH:
try:
branches = [self.request.data['ref'].replace('refs/heads/', '')]
# GitLab only returns one branch.
branch = {
'name': self.request.data['ref'].replace('refs/heads/', ''),
Copy link
Member

Choose a reason for hiding this comment

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

Won't this interferes with your other PR at #3546?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just when merging/rebasing, on the logic everything would work as expected :)

# Assuming the first element is the last commit.
'last_commit': self.request.data['commits'][0],
}
branches = [branch]
Copy link
Member

Choose a reason for hiding this comment

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

Another thought here... If we already know at this point the last commit message, shouldn't skip here the call to self.get_response_push instead of going deeper in the path?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need the (buid, not_building) from the build_branches function to make the response.

return self.get_response_push(self.project, branches)
except KeyError:
raise ParseError('Parameter "ref" is required')
Expand All @@ -211,6 +238,11 @@ class BitbucketWebhookView(WebhookMixin, APIView):
"changes": [{
"new": {
"name": "branch-name",
"target": {
"hash": "sha",
"message": "Update README.md",
...
}
...
},
...
Expand All @@ -219,6 +251,9 @@ class BitbucketWebhookView(WebhookMixin, APIView):
},
...
}

See full payload here:
https://confluence.atlassian.com/bitbucket/event-payloads-740262817.html#EventPayloads-Push
"""

integration_type = Integration.BITBUCKET_WEBHOOK
Expand All @@ -232,12 +267,32 @@ def handle_webhook(self):
if event == BITBUCKET_PUSH:
try:
changes = self.request.data['push']['changes']
branches = [change['new']['name']
for change in changes]
branches = [
{
'name': change['new']['name'],
'last_commit': self._normalize_commit(
change['new']['target']
),
}
for change in changes
]
return self.get_response_push(self.project, branches)
except KeyError:
raise ParseError('Invalid request')

def _normalize_commit(self, commit):
"""
All commit dicts must have this elements at least::
{
'id': 'sha',
'message': 'Update README.md',
}
"""
return {
'id': commit['hash'],
'message': commit['message'],
}


class IsAuthenticatedOrHasToken(permissions.IsAuthenticated):

Expand All @@ -264,6 +319,19 @@ class APIWebhookView(WebhookMixin, APIView):
{
"branches": ["master"]
}

Or the following JSON::

{
"branches": [{
"name": "branch-name",
"last_commit": {
"id": "sha",
"message": "Update README.md"
}
}]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what I proposed for the general webhook, maintaining the backwards compatibility.

"""

integration_type = Integration.API_WEBHOOK
Expand Down
42 changes: 31 additions & 11 deletions readthedocs/rtd_tests/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,9 +339,16 @@ def setUp(self):
def test_github_webhook(self, trigger_build):
"""GitHub webhook API."""
client = APIClient()
payload = {
'ref': 'master',
'head_commit': {
'id': '3eea78b2',
'message': 'Update README.md',
},
}
client.post(
'/api/v2/webhook/github/{0}/'.format(self.project.slug),
{'ref': 'master'},
payload,
format='json',
)
trigger_build.assert_has_calls(
Expand Down Expand Up @@ -369,9 +376,17 @@ def test_github_invalid_webhook(self, trigger_build):
def test_gitlab_webhook(self, trigger_build):
"""GitLab webhook API."""
client = APIClient()
payload = {
'object_kind': 'push',
'ref': 'master',
'commits': [{
'id': '3eea78b2',
'message': 'Update README.md',
}],
}
client.post(
'/api/v2/webhook/gitlab/{0}/'.format(self.project.slug),
{'object_kind': 'push', 'ref': 'master'},
payload,
format='json',
)
trigger_build.assert_has_calls(
Expand All @@ -398,17 +413,22 @@ def test_gitlab_invalid_webhook(self, trigger_build):
def test_bitbucket_webhook(self, trigger_build):
"""Bitbucket webhook API."""
client = APIClient()
client.post(
'/api/v2/webhook/bitbucket/{0}/'.format(self.project.slug),
{
'push': {
'changes': [{
'new': {
'name': 'master',
payload = {
'push': {
'changes': [{
'new': {
'name': 'master',
'target': {
'hash': '3eea78b2',
'message': 'Update README.md',
},
}],
},
},
}],
},
}
client.post(
'/api/v2/webhook/bitbucket/{0}/'.format(self.project.slug),
payload,
format='json',
)
trigger_build.assert_has_calls(
Expand Down
23 changes: 17 additions & 6 deletions readthedocs/rtd_tests/tests/test_integrations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,24 @@ def test_exchange_json_request_body(self):
integration = fixture.get(Integration, project=project,
integration_type=Integration.GITHUB_WEBHOOK,
provider_data='')
resp = client.post(
payload_dict = {
'head_commit': {
'id': '3eea78b2',
'message': 'Update README.md',
},
'ref': 'exchange_json',
}
payload_json = '{"head_commit": {"id": "3eea78b2", "message": "Update README.md"}, "ref": "exchange_json"}'

client.post(
'/api/v2/webhook/github/{0}/'.format(project.slug),
{'ref': 'exchange_json'},
payload_dict,
format='json'
)
exchange = HttpExchange.objects.get(integrations=integration)
self.assertEqual(
exchange.request_body,
'{"ref": "exchange_json"}'
payload_json
)
self.assertEqual(
exchange.request_headers,
Expand All @@ -62,15 +71,17 @@ def test_exchange_form_request_body(self):
integration = fixture.get(Integration, project=project,
integration_type=Integration.GITHUB_WEBHOOK,
provider_data='')
resp = client.post(
payload_decode = '{"head_commit": {"id": "3eea78b2", "message": "Update README.md"}, "ref": "exchange_form"}'
payload_encode = '%7B%22head_commit%22%3A%20%7B%22id%22%3A%20%223eea78b2%22%2C%20%22message%22%3A%20%22Update%20README.md%22%7D%2C%20%22ref%22%3A%20%22exchange_form%22%7D'
Copy link
Member Author

Choose a reason for hiding this comment

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

I use this tool for encode/decode https://meyerweb.com/eric/tools/dencoder/

Note: the order matters!

Copy link
Member

Choose a reason for hiding this comment

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

Can you do this with a Python function to encode a JSON into an URL query string? (it should be there one in the standard library)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I think that's cleaner, just wanted to keep to the current tests style.

client.post(
'/api/v2/webhook/github/{0}/'.format(project.slug),
'payload=%7B%22ref%22%3A+%22exchange_form%22%7D',
'payload={}'.format(payload_encode),
content_type='application/x-www-form-urlencoded',
)
exchange = HttpExchange.objects.get(integrations=integration)
self.assertEqual(
exchange.request_body,
'{"ref": "exchange_form"}'
payload_decode
)
self.assertEqual(
exchange.request_headers,
Expand Down