Allow to skip a build on commit messages #3457

stsewd · 2017-12-28T17:55:13Z

This implements a feature discussed here #871

When a webhook is called, its receive the last commit after the push event,
the following marks are searched in the commit message:

[skip doc]
[skip docs]
[docs skip]
[doc skip]

If one is found, the build process is skipped.

Still wip, this is what I have so far:

Marks proposed: - [skip doc] - [skip docs] - [docs skip] - [doc skip]

stsewd · 2017-12-28T17:58:09Z

readthedocs/restapi/views/integrations.py

@@ -159,7 +167,13 @@ def handle_webhook(self):
        # Handle push events and trigger builds
        if event == GITHUB_PUSH:
            try:
-                branches = [self.data['ref'].replace('refs/heads/', '')]
+                branch = {


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

stsewd · 2017-12-28T17:59:15Z

readthedocs/restapi/views/integrations.py

+                branch = {
+                    'name': self.request.data['ref'].replace('refs/heads/', ''),
+                    # Assuming the first element is the last commit.
+                    'commit': self.request.data['commits'][0],


Not sure about this, didn't found much documentation about this on gitlab.

stsewd · 2017-12-28T18:00:45Z

readthedocs/restapi/views/integrations.py

+                }
+            }]
+        }
+


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

stsewd · 2017-12-28T18:02:00Z

readthedocs/core/views/hooks.py

+                version.slug
+                for version in versions
+            }
+            return (to_build, not_building)


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

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

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.

stsewd · 2017-12-28T18:12:42Z

readthedocs/restapi/views/integrations.py

            ...
        }
+
+    See full payload here:
+    https://developer.github.com/v3/activity/events/types/#pushevent


Are we using the v3 of the gh api?

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

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

Tests from generic aren't updated for now

Json and urlencoded test case

stsewd · 2017-12-29T04:57:23Z

readthedocs/rtd_tests/tests/test_integrations.py

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


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

Note: the order matters!

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)

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

stsewd · 2017-12-29T05:26:33Z

While updating some tests, I found that there is some old webhook code (https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/core/views/hooks.py#L158) that is marked as deprecated, should we remove this code? (another PR?) o should we still support the v1 webhooks?

humitos · 2018-02-15T22:49:57Z

that is marked as deprecated, should we remove this code? (another PR?) o should we still support the v1 webhooks?

Hehe, I asked the same to Eric and he checked the logs: many people still use this so we can't remove this endpoint :(

humitos · 2018-02-15T22:51:50Z

readthedocs/core/views/hooks.py

@@ -66,6 +66,23 @@ def _build_version(project, slug, already_built=()):
        return None


+def _contains_skip_mark(commit):


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

humitos · 2018-02-15T22:53:24Z

readthedocs/core/views/hooks.py

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


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

humitos

I like the addition of this new feature.

Although, I think it needs more work and make some decision on the logic/flow followed when a webhook is received. It's still kind of complex to me how the path is and "where this code should live" (the skip you wrote).

Please, take a look at my comments and let's keep discussing where and how :)

humitos · 2018-02-15T23:01:38Z

readthedocs/core/views/hooks.py

+                version.slug
+                for version in versions
+            }
+            return (to_build, not_building)


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

humitos · 2018-02-15T23:04:28Z

readthedocs/restapi/views/integrations.py

            ...
        }
+
+    See full payload here:
+    https://developer.github.com/v3/activity/events/types/#pushevent


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

humitos · 2018-02-15T23:06:46Z

readthedocs/core/views/hooks.py

@@ -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'])


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

See #3457 (comment)

humitos · 2018-02-15T23:08:27Z

readthedocs/restapi/views/integrations.py

-                branches = [self.request.data['ref'].replace('refs/heads/', '')]
+                # GitLab only returns one branch.
+                branch = {
+                    'name': self.request.data['ref'].replace('refs/heads/', ''),


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

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

humitos · 2018-02-15T23:09:36Z

readthedocs/restapi/views/integrations.py

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


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?

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

humitos · 2018-02-15T23:14:14Z

readthedocs/rtd_tests/tests/test_integrations.py

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


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)

humitos · 2018-02-15T23:18:20Z

Also, if you can, please add test cases for this specific feature so we can be sure that it works and then we are safe while refactoring/restructuring the code.

stsewd · 2018-02-18T21:10:05Z

@humitos thanks for the feedback, I have improve the log and added some tests, I also made sure to keep the changes backwards compatible with the old webhooks and with the generic webhook.

stsewd · 2018-04-11T03:23:24Z

Some previous tests were wrong and pass unnoticed because of the old mock library. Everything is correct now (I think).

ericholscher · 2018-11-02T16:19:40Z

Feels like this has a larger refactor in it. Is this still something useful that we should try and get in?

stsewd · 2018-11-02T18:12:54Z

Feels like this has a larger refactor in it. Is this still something useful that we should try and get in?

Hmm, not sure. This will allow users to help us to save some resources, but I don't know how much.

humitos · 2018-11-06T09:44:39Z

This is "nice to have" but I'm not sure that it worth to add this complexity to the code. Also, I'm not sure how many people will use this feature in the end since it's something manually that also could "mess/dirty" the commit messages from the repo.

humitos · 2019-01-09T17:09:28Z

I'm closing this PR here because it was off our roadmap and we consider that adding this complexity to the code base for a feature that will affect more our servers than the user itself does not worth it.

Thanks a lot for this code and I'm sorry that it's not going to be merged 😞

stsewd added 5 commits December 28, 2017 12:35

Change GitHub webhook

f6d9780

Change GitLab webhook

e851bf0

Change BitBucket webhook

e21af7e

Change spec for general webhook

634a6ed

Validate for commits containing a skip mark

3eea78b

Marks proposed: - [skip doc] - [skip docs] - [docs skip] - [doc skip]

stsewd commented Dec 28, 2017

View reviewed changes

stsewd added 4 commits December 28, 2017 23:49

Fix comments

177f0f7

Fix wrong key

f20fcd3

Update tests from test_api.py

8150793

Tests from generic aren't updated for now

Update test_integrations.py

1c89e6a

Json and urlencoded test case

stsewd commented Dec 29, 2017

View reviewed changes

humitos added the PR: work in progress Pull request is not ready for full review label Dec 29, 2017

humitos reviewed Feb 15, 2018

View reviewed changes

humitos requested changes Feb 15, 2018

View reviewed changes

stsewd added 9 commits February 18, 2018 12:20

Better log message

64e00b0

Use requests for encode payload

435726c

Merge branch 'master' into allow-skip-build

24dd729

Check commit message no object

a930dc3

Update test payload

517d3eb

Make generic webhook backwards compatible

8ab67be

Update payload

12147c8

Set webhooks from api v1 compatible

b808ce4

Linter

c8b8756

Add tests

836c9ca

stsewd added 5 commits February 18, 2018 18:51

Update generic API parameters

806789d

Add details to skip a build

97235a3

Change list -> array

5803557

Merge branch 'master' into allow-skip-build

ce0fa0f

Fix tests

808f835

stsewd added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Apr 11, 2018

agjohnson removed the PR: ready for review label Jun 8, 2018

stsewd added the Needed: design decision A core team decision is required label Nov 21, 2018

humitos closed this Jan 9, 2019

humitos mentioned this pull request Jan 9, 2019

Can i mark a commit as to skip building the docs? #871

Closed

stsewd deleted the allow-skip-build branch January 9, 2019 17:26

saadmk11 mentioned this pull request Aug 7, 2019

Add Configuration for PR Builder Through Config File #6046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to skip a build on commit messages #3457

Allow to skip a build on commit messages #3457

stsewd commented Dec 28, 2017 •

edited

Loading

stsewd Dec 28, 2017

stsewd Dec 28, 2017 •

edited

Loading

stsewd Dec 28, 2017

stsewd Dec 28, 2017

humitos Feb 15, 2018

stsewd Feb 18, 2018

stsewd Dec 28, 2017

humitos Feb 15, 2018

stsewd Feb 18, 2018 •

edited

Loading

stsewd Dec 29, 2017

humitos Feb 15, 2018

stsewd Feb 18, 2018

stsewd commented Dec 29, 2017 •

edited

Loading

humitos commented Feb 15, 2018

humitos Feb 15, 2018

humitos Feb 15, 2018

humitos left a comment

humitos Feb 15, 2018

humitos Feb 15, 2018

humitos Feb 15, 2018

stsewd Feb 18, 2018

humitos Feb 15, 2018

stsewd Feb 18, 2018

humitos Feb 15, 2018

stsewd Feb 18, 2018

humitos Feb 15, 2018

humitos commented Feb 15, 2018

stsewd commented Feb 18, 2018 •

edited

Loading

stsewd commented Apr 11, 2018

ericholscher commented Nov 2, 2018

stsewd commented Nov 2, 2018

humitos commented Nov 6, 2018

humitos commented Jan 9, 2019 •

edited

Loading

		@@ -66,6 +66,23 @@ def _build_version(project, slug, already_built=()):
		return None


		def _contains_skip_mark(commit):

Allow to skip a build on commit messages #3457

Allow to skip a build on commit messages #3457

Conversation

stsewd commented Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

stsewd Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Dec 29, 2017 • edited Loading

humitos commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Feb 15, 2018

stsewd commented Feb 18, 2018 • edited Loading

stsewd commented Apr 11, 2018

ericholscher commented Nov 2, 2018

stsewd commented Nov 2, 2018

humitos commented Nov 6, 2018

humitos commented Jan 9, 2019 • edited Loading

stsewd commented Dec 28, 2017 •

edited

Loading

stsewd Dec 28, 2017 •

edited

Loading

stsewd Feb 18, 2018 •

edited

Loading

stsewd commented Dec 29, 2017 •

edited

Loading

stsewd commented Feb 18, 2018 •

edited

Loading

humitos commented Jan 9, 2019 •

edited

Loading