Fix occasionally wrong change url with github

Github returns different urls for pull requests. One for api use and
one for browser use. The change url we're interested in is the one for
browser usage. However zuul takes the wrong one from the pull request
data structure but most of the time overwrites the change url
generated from event metadata. This leads to the issue that the change
url is sometimes not correct.

This can be fixed by taking the html_url from the pull request data
structure. Further due to the fact that the url is always set on a pr
and stays static throughout the whole lifecycle of the pr, be safe and
just don't overwrite the url from event metadata.

Change-Id: I2030b9dddd5bc618231b73d73ae64e2552231769

tests/fakegithub.py

@@ -367,7 +367,10 @@ class FakePull(object):
         data = {
             'number': pr.number,
             'title': pr.subject,
-            'url': 'https://%s/%s/pull/%s' % (
+            'url': 'https://%s/api/v3/%s/pulls/%s' % (
+                connection.server, pr.project, pr.number
+            ),
+            'html_url': 'https://%s/%s/pull/%s' % (
                 connection.server, pr.project, pr.number
             ),
             'updated_at': pr.updated_at,

tests/fixtures/config/inventory/git/common-config/zuul.yaml

@@ -5,12 +5,17 @@
     trigger:
       gerrit:
         - event: patchset-created
+      github:
+        - event: pull_request
+          action: opened
     success:
       gerrit:
         Verified: 1
+      github: {}
     failure:
       gerrit:
         Verified: -1
+      github: {}
 
 - nodeset:
     name: nodeset1

tests/fixtures/config/inventory/git/org_project3/.zuul.yaml

@@ -0,0 +1,10 @@
+- project:
+    check:
+      jobs:
+        - single-inventory
+        - single-inventory-list
+        - executor-only-inventory
+        - group-inventory
+        - hostvars-inventory
+        - ansible-version27-inventory
+        - ansible-version28-inventory

tests/fixtures/config/inventory/git/org_project3/README

@@ -0,0 +1 @@
+test

tests/fixtures/config/inventory/main.yaml

@@ -7,3 +7,6 @@
         untrusted-projects:
           - org/project
           - org/project2
+      github:
+        untrusted-projects:
+          - org/project3

tests/fixtures/zuul-winrm.conf

@@ -25,6 +25,10 @@ server=review.example.com
 user=jenkins
 sshkey=fake_id_rsa_path
 
+[connection github]
+driver=github
+webhook_token=0000000000000000000000000000000000000000
+
 [connection smtp]
 driver=smtp
 server=localhost

tests/unit/test_inventory.py

@@ -23,15 +23,23 @@ from tests.base import ZuulTestCase
 
 class TestInventoryBase(ZuulTestCase):
 
+    config_file = 'zuul-gerrit-github.conf'
     tenant_config_file = 'config/inventory/main.yaml'
+    use_gerrit = True
 
     def setUp(self, python_path=None):
         super(TestInventoryBase, self).setUp()
         if python_path:
             self.fake_nodepool.python_path = python_path
         self.executor_server.hold_jobs_in_build = True
-        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
-        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        if self.use_gerrit:
+            A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+            self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        else:
+            A = self.fake_github.openFakePullRequest(
+                'org/project3', 'master', 'A')
+            self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
+
         self.waitUntilSettled()
 
     def _get_build_inventory(self, name):
@@ -46,6 +54,39 @@ class TestInventoryBase(ZuulTestCase):
         return yaml.safe_load(open(setup_inv_path, 'r'))
 
 
+class TestInventoryGithub(TestInventoryBase):
+
+    use_gerrit = False
+
+    def test_single_inventory(self):
+
+        inventory = self._get_build_inventory('single-inventory')
+
+        all_nodes = ('ubuntu-xenial',)
+        self.assertIn('all', inventory)
+        self.assertIn('hosts', inventory['all'])
+        self.assertIn('vars', inventory['all'])
+        for node_name in all_nodes:
+            self.assertIn(node_name, inventory['all']['hosts'])
+            node_vars = inventory['all']['hosts'][node_name]
+            self.assertEqual(
+                'auto', node_vars['ansible_python_interpreter'])
+        self.assertIn('zuul', inventory['all']['vars'])
+        self.assertIn('attempts', inventory['all']['vars']['zuul'])
+        self.assertEqual(1, inventory['all']['vars']['zuul']['attempts'])
+        z_vars = inventory['all']['vars']['zuul']
+        self.assertIn('executor', z_vars)
+        self.assertIn('src_root', z_vars['executor'])
+        self.assertIn('job', z_vars)
+        self.assertEqual(z_vars['job'], 'single-inventory')
+        self.assertEqual(z_vars['message'], 'QQ==')
+        self.assertEqual(z_vars['change_url'],
+                         'https://github.com/org/project3/pull/1')
+
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+
 class TestInventoryPythonPath(TestInventoryBase):
 
     def setUp(self):
@@ -294,6 +335,7 @@ class TestInventory(TestInventoryBase):
 
 class TestAnsibleInventory(AnsibleZuulTestCase):
 
+    config_file = 'zuul-gerrit-github.conf'
     tenant_config_file = 'config/inventory/main.yaml'
 
     def _get_file(self, build, path):

zuul/driver/github/githubconnection.py

@@ -1045,16 +1045,6 @@ class GithubConnection(BaseConnection):
             change = self._getChange(project, event.change_number,
                                      event.patch_number, refresh=refresh,
                                      event=event)
-            if hasattr(event, 'change_url') and event.change_url:
-                change.url = event.change_url
-            else:
-                # The event has no change url so just construct it
-                change.url = self.getPullUrl(
-                    event.project_name, event.change_number)
-            change.uris = [
-                'https://%s/%s/pull/%s' % (
-                    self.server, project, change.number),
-            ]
             change.source_event = event
             change.is_current_patchset = (change.pr.get('head').get('sha') ==
                                           event.patch_number)
@@ -1261,7 +1251,12 @@ class GithubConnection(BaseConnection):
         if not change.updated_at:
             change.updated_at = self._ghTimestampToDate(
                 change.pr.get('updated_at'))
-        change.url = change.pr.get('url')
+
+        # Note: Github returns different urls for the pr:
+        #  - url: this is the url meant for api use
+        #  - html_url: this is the url meant for use in browser (this is what
+        #              change.url means)
+        change.url = change.pr.get('html_url')
         change.uris = [
             'https://%s/%s/pull/%s' % (
                 self.server, change.project.name, change.number),