Allow tags from GitHub webhooks #3546

stsewd · 2018-01-24T16:43:33Z

This closes #3508

This is just a proposal, since I'm not sure if this is the best way of do it (since various methods are named branch* and no tags o versions, e.g. versions_from_branch_name)

The GitHub payload looks like this when a tag is pushed:

{
  "ref": "refs/tags/v02",
  ...
}

If the tag isn't registered on rtd active versions (recent created tags), the build isn't triggered; but if the tag is forced on the gh repo and the tag is an active version, the build is triggered.

ericholscher · 2018-01-30T01:59:35Z

This looks like a great PR. It needs to at least have a test that shows what wasn't working before, but does work now. I'm +1 on it though!

stsewd · 2018-01-31T23:40:50Z

Well, the current tests didn't really check this functionality, ~~I'm not sure how to really test this~~.

stsewd · 2018-01-31T23:45:29Z

readthedocs/rtd_tests/tests/test_api.py

@@ -335,17 +335,20 @@ class IntegrationsTests(TestCase):
    def setUp(self):
        self.project = get(Project)
        self.version = get(Version, verbose_name='master', project=self.project)
+        self.version_tag = get(Version, verbose_name='v1.2', project=self.project)


This really doesn't make any difference to the current tests

stsewd · 2018-01-31T23:46:50Z

readthedocs/rtd_tests/tests/test_api.py

+            {'ref': 'refs/tags/v1.2'},
+            format='json',
+        )
+        trigger_build.assert_has_calls(


This just test if the method was called, no if the branch/tag was correctly parsed

stsewd · 2018-02-01T20:47:56Z

@ericholscher After fighting with the mocks, the tests are ready 😊

stsewd · 2018-02-01T21:27:55Z

readthedocs/rtd_tests/tests/test_api.py

+        trigger_build.assert_has_calls(
+            [mock.call(force=True, version=self.version, project=self.project)])
+
+    @mock.patch('readthedocs.restapi.views.integrations.WebhookMixin.get_response_push', return_value=None)


Here the return value is None, since we only care to test if the ref value is parsed correctly, the other test covers the all flow

Since what you want to test is the parsing, instead of writing the test in this way, why not avoid mocking this function and calling it for real and assert the return value with the one that it's expected?

Like this:

wm = WebhookMixin() assertEqual(wm._normalize_ref('refs/heads/master'), 'master')

It's clearer this way.

I wasn't sure if that way was the correct one 😞.

humitos

Thanks for this PR!

I didn't find anything that it's wrong, but I left some suggestions on the comments.

humitos · 2018-02-15T21:52:00Z

readthedocs/projects/models.py

@@ -751,7 +751,8 @@ def versions_from_branch_name(self, branch):
        return (
            self.versions.filter(identifier=branch) |
            self.versions.filter(identifier='remotes/origin/%s' % branch) |
-            self.versions.filter(identifier='origin/%s' % branch)
+            self.versions.filter(identifier='origin/%s' % branch) |
+            self.versions.filter(verbose_name=branch)


I think this could return duplicated values when the identifier and the verbose name are the same, right? Maybe we should use here the .distinct()

On the other hand, I don't follow why we need to add this line here, hehe. Can you clarify?

Tags are saved this way: {identifier: hash, verbose_name: tagname}.

GitHub only provides the tag name on the ref.

The only case were I think duplicate values may occur is for branches, since the identifier isn't a hash like they are for tags. I'm going to research a little.

These is what I found, looks like the querysets are really sets p:

>>> p.versions.filter(identifier=branch) [<Version: Version awesome of Read The Docs (8)>] >>> p.versions.filter(verbose_name=branch) [<Version: Version awesome of Read The Docs (8)>] >>> p.versions.filter(identifier=branch) | p.versions.filter(verbose_name=branch) [<Version: Version awesome of Read The Docs (8)>]

They aren't really sets. In fact, that's why the distinct is: https://docs.djangoproject.com/en/2.0/ref/models/querysets/#django.db.models.query.QuerySet.distinct

Anyway, it seems that we are not bitten by this problem here.

humitos · 2018-02-15T21:57:16Z

readthedocs/rtd_tests/tests/test_api.py

-            [mock.call(force=True, version=mock.ANY, project=self.project)])
+            [mock.call(force=True, version=self.version, project=self.project)])
+
+        client.post(


Write this as a separte test for tag and rename this one for branch since they the branch was working previously and we are adding a new feature. It's good to each test written to try a specific thing.

humitos · 2018-02-15T22:01:44Z

readthedocs/rtd_tests/tests/test_api.py

+        trigger_build.assert_has_calls(
+            [mock.call(force=True, version=self.version, project=self.project)])
+
+    @mock.patch('readthedocs.restapi.views.integrations.WebhookMixin.get_response_push', return_value=None)


Since what you want to test is the parsing, instead of writing the test in this way, why not avoid mocking this function and calling it for real and assert the return value with the one that it's expected?

Like this:

wm = WebhookMixin() assertEqual(wm._normalize_ref('refs/heads/master'), 'master')

It's clearer this way.

humitos · 2018-02-15T22:03:06Z

readthedocs/rtd_tests/tests/test_api.py

+
+        client.post(
+            '/api/v2/webhook/github/{0}/'.format(self.project.slug),
+            {'ref': 'refs/tags/v1.0'},


Same here: move this cases to a specific test.

stsewd · 2018-02-16T02:46:31Z

@humitos all corrections were made, thanks again!

humitos

Good job! It's ready to be merged for me.

stsewd · 2018-04-11T03:42:20Z

Nothing change on this PR, just rebase to the current master :)

RichardLitt added Improvement Minor improvement to code PR: ready for review labels Jan 25, 2018

RichardLitt added the Needed: tests Tests are required label Jan 30, 2018

stsewd force-pushed the allow-tags-from-gh branch from 8ca0809 to 0859712 Compare January 31, 2018 22:14

stsewd commented Jan 31, 2018

View reviewed changes

stsewd commented Feb 1, 2018

View reviewed changes

humitos reviewed Feb 15, 2018

View reviewed changes

humitos mentioned this pull request Feb 15, 2018

Allow to skip a build on commit messages #3457

Closed

7 tasks

humitos approved these changes Feb 20, 2018

View reviewed changes

RichardLitt removed the Needed: tests Tests are required label Feb 21, 2018

stsewd added 9 commits April 10, 2018 22:33

Filter by verbose_name

f00d144

Allow tags from GitHub webhook

e296f00

Add tests

b65bb12

Remove useless instance

ace010a

Add real tests for parsing ref

7fb1ddb

Fix versions

b169cc9

Check for versions

3ba5403

Refactor tests

0c7433d

Split tests

49fadeb

stsewd force-pushed the allow-tags-from-gh branch from 8586e29 to 49fadeb Compare April 11, 2018 03:33

stsewd mentioned this pull request Apr 20, 2018

Add travis-ci style pull request builder #1340

Closed

dlech mentioned this pull request Jun 5, 2018

ReadTheDocs does not pull tags #3508

Closed

Merge branch 'master' into allow-tags-from-gh

2270cce

agjohnson added this to the 2.6 milestone Jun 8, 2018

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

agjohnson merged commit 07bcf4c into readthedocs:master Jun 8, 2018

stsewd deleted the allow-tags-from-gh branch June 8, 2018 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow tags from GitHub webhooks #3546

Allow tags from GitHub webhooks #3546

stsewd commented Jan 24, 2018

ericholscher commented Jan 30, 2018

stsewd commented Jan 31, 2018 •

edited

Loading

stsewd Jan 31, 2018

stsewd Jan 31, 2018 •

edited

Loading

stsewd commented Feb 1, 2018 •

edited

Loading

stsewd Feb 1, 2018

humitos Feb 15, 2018

stsewd Feb 16, 2018

humitos left a comment

humitos Feb 15, 2018

stsewd Feb 16, 2018

stsewd Feb 16, 2018

humitos Feb 16, 2018

humitos Feb 15, 2018

humitos Feb 15, 2018

humitos Feb 15, 2018

stsewd commented Feb 16, 2018

humitos left a comment

stsewd commented Apr 11, 2018

Allow tags from GitHub webhooks #3546

Allow tags from GitHub webhooks #3546

Conversation

stsewd commented Jan 24, 2018

ericholscher commented Jan 30, 2018

stsewd commented Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

stsewd Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

stsewd commented Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

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

stsewd commented Feb 16, 2018

humitos left a comment

Choose a reason for hiding this comment

stsewd commented Apr 11, 2018

stsewd commented Jan 31, 2018 •

edited

Loading

stsewd Jan 31, 2018 •

edited

Loading

stsewd commented Feb 1, 2018 •

edited

Loading