diff options
33 files changed, 586 insertions, 85 deletions
diff --git a/.zuul.yaml b/.zuul.yaml index a2c34b51e..913d34965 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -130,8 +130,8 @@ allowed-projects: zuul/zuul timeout: 2700 # 45 minutes requires: - - python-builder-3.8-container-image - - python-base-3.8-container-image + - python-builder-3.7-container-image + - python-base-3.7-container-image provides: zuul-container-image vars: &zuul_image_vars docker_images: @@ -160,8 +160,8 @@ description: Build Docker images and upload to Docker Hub. allowed-projects: zuul/zuul requires: - - python-builder-3.8-container-image - - python-base-3.8-container-image + - python-builder-3.7-container-image + - python-base-3.7-container-image provides: zuul-container-image secrets: name: docker_credentials diff --git a/Dockerfile b/Dockerfile index 96c323217..e1a30d5b3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -FROM docker.io/opendevorg/python-builder:3.8 as builder +FROM docker.io/opendevorg/python-builder:3.7 as builder # Optional location of Zuul API endpoint. ARG REACT_APP_ZUUL_API @@ -39,7 +39,7 @@ RUN mkdir /tmp/openshift-install \ && echo $OPENSHIFT_SHA /tmp/openshift-install/openshift-client.tgz | sha256sum --check \ && tar xvfz openshift-client.tgz --strip-components=1 -C /tmp/openshift-install -FROM docker.io/opendevorg/python-base:3.8 as zuul +FROM docker.io/opendevorg/python-base:3.7 as zuul COPY --from=builder /output/ /output RUN /output/install-from-bindep \ diff --git a/bindep.txt b/bindep.txt index 62bd00c4b..5c1641660 100644 --- a/bindep.txt +++ b/bindep.txt @@ -34,7 +34,7 @@ libyaml [platform:redhat] libyaml-dev [platform:dpkg compile test] libyaml-devel [platform:rpm compile test] gmp [platform:apk] -procps [platform:apk] +procps [platform:apk platform:dpkg] python3-dev [compile test platform:dpkg platform:apk] python3-devel [compile test platform:rpm] python3.7 [test platform:ubuntu-bionic] diff --git a/doc/source/discussion/github-checks-api.rst b/doc/source/discussion/github-checks-api.rst new file mode 100644 index 000000000..73788a5d2 --- /dev/null +++ b/doc/source/discussion/github-checks-api.rst @@ -0,0 +1,170 @@ +:title: Github Checks API + +Github Checks API +================= + +Using the `Github Checks API`_ to report job results back to a PR provides +some additional features compared to the status API like file comments and +custom actions. The latter one could be used to e.g. cancel a running +build. + +Design decisions +----------------- + +The github checks API consists mainly of two entities: `Check Suites`_ and +`Check Runs`_. Check suites are a collection of check runs for a specific +commit and summarize their status and conclusion. + +Following this description, one might think that the check suite is a +perfect mapping for a pipeline execution in zuul and a check run could map +to a single job execution that is part of the pipeline run. Unfortunately, +there are a few restrictions that don't allow this kind of mapping. + +First of all, check suites are completely managed by Github. Apart from +creating a check suite for a commit SHA, we can't do anything with it. +The current status, duration and the conclusion are all calculated and +set by Github automatically whenever an included check run is updated. + +There can only be one check suite per commit sha per app. Thus, even if +we could update the check suite, we wouldn't be able to create one check +suite for each pipeline, e.g. check and gate. + +When configuring the branch protection in Github, only a check run can +be selected as required status check. Having each job as a dedicated +check run would result in a huge list of status checks one would have to +enable to make the branch protection work. Additionally, we would then +loose some of Zuul's features like non-voting jobs and it would break +Zuul's gating capabilities as they are working on a pipeline level, not on +a job level. + +Zuul can only report the whole buildset, but no individual jobs. With +that we wouldn't be able to update individual check runs on a job level. + +Having said the above, the only possible integration of the checks API is +on a pipeline level, so each pipeline execution maps to a check run in +Github. + +Behaviour in Zuul +----------------- + +Reporting +~~~~~~~~~ + +The Github reporter is able to report both a status +:attr:`pipeline.<reporter>.<github source>.status` or a check +:attr:`pipeline.<reporter>.<github source>.check`. While it's possible to +configure a Github reporter to report both, it's recommended to use only one. +Reporting both might result in duplicated status check entries in the Github +PR (the section below the comments). + +Trigger +~~~~~~~ + +The Github driver is able to trigger on a reported check +(:value:`pipeline.trigger.<github source>.event.check_run`) similar to a +reported status (:value:`pipeline.trigger.<github source>.action.status`). + +Requirements +~~~~~~~~~~~~ + +While trigger and reporter differentiates between status and check, the Github +driver does not differentiate between them when it comes to pipeline +requirements. This is mainly because Github also doesn't differentiate between +both in terms of branch protection and `status checks`_. + +Actions / Events +---------------- + +Github provides a set of default actions for check suites and check runs. +Those actions are available as buttons in the Github UI. Clicking on those +buttons will emit webhook events which will be handled by Zuul. + +These actions are only available on failed check runs / check suites. So +far, a running or successful check suite / check run does not provide any +action from Github side. + +Available actions are: + +Re-run all checks + Github emits a webhook event with type ``check_suite`` and action + ``rerequested`` that is meant to re-run all check-runs contained in this + check suite. Github does not provide the list of check-runs in that case, + so it's up to the Github app what should run. + +Re-run failed checks + Github emits a webhook event with type ``check_run`` and action + ``rerequested`` for each failed check run contained in this suite. + +Re-run + Github emits a webhook event with type ``check_run`` and action + ``rerequested`` for the specific check run. + +Zuul will handle all events except for the `Re-run all checks` event as +this is not suitable for the Zuul workflow as it doesn't make sense to +trigger all pipelines to run simultaniously. + +The drawback here is, that we are not able to customize those events in Github. +Github will always say "You have successfully requested ..." although we aren't +listening to the event at all. Therefore, it might be a solution to handle the +`Re-run all checks` event in Zuul similar to `Re-run failed checks` just to +not do anything while Github makes the user believe an action was really +triggered. + + +File comments (annotations) +--------------------------- + +Check runs can be used to post file comments directly in the files of the PR. +Those are similar to user comments, but must provide some more information. + +Zuul jobs can already return file comments via ``zuul_return`` +(see: :ref:`return_values`). We can simply use this return value, build the +necessary annotations (how Github calls it) from it and attach them to the +check run. + + +Custom actions +~~~~~~~~~~~~~~ + +Check runs can provide some custom actions which will result in additional +buttons being available in the Github UI for this specific check run. +Clicking on such a button will emit a webhook event with type ``check_run`` +and action ``requested_action`` and will additionally contain the id/name of +the requested action which we can define when creating the action on the +check run. + +We could use these custom actions to provide some "Re-run" action on a +running check run (which might otherwise be stuck in case a check run update +fails) or to abort a check run directly from the Github UI. + + +Restrictions and Recommendations +-------------------------------- + +Although both the checks API and the status API can be activated for a +Github reporter at the same time, it's not recommmended to do so as this might +result in multiple status checks to be reported to the PR for the same pipeline +execution (which would result in duplicated entries in the status section below +the comments of a PR). + +In case the update on a check run fails (e.g. request timeout when reporting +success or failure to Github), the check run will stay in status "in_progess" +and there will be no way to re-run the check run via the Github UI as the +predefined actions are only available on failed check runs. +Thus, it's recommended to configure a +:value:`pipeline.trigger.<github source>.action.comment` trigger on the +pipeline to still be able to trigger re-run of the stuck check run via e.g. +"recheck". + +The check suite will only list check runs that were reported by Zuul. If +the requirements for a certain pipeline are not met and it is not run, the +check run for this pipeline won't be listed in the check suite. However, +this does not affect the required status checks. If the check run is enabled +as required, Github will still show it in the list of required status checks +- even if it didn't run yet - just not in the check suite. + + +.. _Github Checks API: https://developer.github.com/v3/checks/ +.. _Check Suites: https://developer.github.com/v3/checks/suites/ +.. _Check Runs: https://developer.github.com/v3/checks/runs/ +.. _status checks: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-status-checks#types-of-status-checks-on-github diff --git a/doc/source/discussion/index.rst b/doc/source/discussion/index.rst index a02b5ff97..efa59d6a9 100644 --- a/doc/source/discussion/index.rst +++ b/doc/source/discussion/index.rst @@ -16,3 +16,4 @@ configure it to meet your needs. gating encryption tenant-scoped-rest-api + github-checks-api diff --git a/doc/source/reference/drivers/github.rst b/doc/source/reference/drivers/github.rst index 474c8ac82..c2c085eca 100644 --- a/doc/source/reference/drivers/github.rst +++ b/doc/source/reference/drivers/github.rst @@ -499,6 +499,21 @@ enqueued into the pipeline. request. The syntax is ``user:status:value``. This can also be a regular expression. + Zuul does not differentiate between a status reported via + status API or via checks API (which is also how Github behaves + in terms of branch protection and `status checks`__). + Thus, the status could be reported by a + :attr:`pipeline.<reporter>.<github source>.status` or a + :attr:`pipeline.<reporter>.<github source>.check`. + + When a status is reported via the status API, Github will add + a ``[bot]`` to the name of the app that reported the status, + resulting in something like ``user[bot]:status:value``. For a + status reported via the checks API, the app's slug will be + used as is. + + .. __: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-status-checks#types-of-status-checks-on-github + .. attr:: label A string value indicating that the pull request must have the diff --git a/doc/source/reference/drivers/pagure.rst b/doc/source/reference/drivers/pagure.rst index 0614ad222..7845ae79a 100644 --- a/doc/source/reference/drivers/pagure.rst +++ b/doc/source/reference/drivers/pagure.rst @@ -24,7 +24,6 @@ Each project to be integrated with Zuul needs: - "Web hook target" set to http://<zuul-web>/zuul/api/connection/<conn-name>/payload -- "Notify on pull-request flag" set to on - "Pull requests" set to on - "Open metadata access to all" set to off (optional, expected if approval based on PR a metadata tag) @@ -32,9 +31,11 @@ Each project to be integrated with Zuul needs: the score requierement (optional, expected if score requierement is defined in a pipeline) -Furthermore, the user must be added as project collaborator **admin** to -be able to read the project's webhook token. This token is used -to validate webhook's payload. +Furthermore, the user must be added as project collaborator +(**ticket** access level), to be able to read the project's +webhook token. This token is used to validate webhook's payload. But +if Zuul is configured to merge pull requests then the access level +must be **commit**. Connection Configuration ------------------------ @@ -81,6 +82,12 @@ The supported options in ``zuul.conf`` connections are: Path to the Pagure Git repositories. Used to clone. + .. attr:: app_name + :default: Zuul + + Display name that will appear as the application name in front + of each CI status flag. + .. attr:: source_whitelist :default: '' diff --git a/releasenotes/notes/ansible-27-deprecated-cd82a8a47a10a8c7.yaml b/releasenotes/notes/ansible-27-deprecated-cd82a8a47a10a8c7.yaml new file mode 100644 index 000000000..d1f9e0c94 --- /dev/null +++ b/releasenotes/notes/ansible-27-deprecated-cd82a8a47a10a8c7.yaml @@ -0,0 +1,5 @@ +--- +upgrade: + - | + Ansible 2.7 is now deprecated since it only receives security updates + and will be end of life soon. diff --git a/releasenotes/notes/github-require-check-294d3f27da790fae.yaml b/releasenotes/notes/github-require-check-294d3f27da790fae.yaml new file mode 100644 index 000000000..a01d59b20 --- /dev/null +++ b/releasenotes/notes/github-require-check-294d3f27da790fae.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + The status pipeline requirements of the Github driver + :attr:`pipeline.require.<github source>.status` now also matches + on statuses reported via the Github checks API. diff --git a/requirements.txt b/requirements.txt index 5f0de0f90..732aa35a8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,7 +2,7 @@ pbr>=1.1.0 # Temporary fix for https://gitlab.com/python-devs/importlib_resources/issues/83 importlib-resources==1.0.2 # Early virtualenv 20 had bad file location assumptions -virtualenv!=20.0.0,!=20.0.1 +virtualenv!=20.0.0,!=20.0.1,>20 github3.py>=1.1.0 PyYAML>=3.1.0 diff --git a/tests/base.py b/tests/base.py index 4a4e536a3..a697be384 100644 --- a/tests/base.py +++ b/tests/base.py @@ -1235,19 +1235,33 @@ class FakePagurePullRequest(object): return self._getPullRequestEvent( 'pull-request.tag.added', pull_data_field='pull_request') - def getPullRequestStatusSetEvent(self, status): + def getPullRequestStatusSetEvent(self, status, username="zuul"): self.addFlag( - status, "https://url", "Build %s" % status) + status, "https://url", "Build %s" % status, username) return self._getPullRequestEvent('pull-request.flag.added') - def addFlag(self, status, url, comment, username="Pingou"): + def insertFlag(self, flag): + to_pop = None + for i, _flag in enumerate(self.flags): + if _flag['uid'] == flag['uid']: + to_pop = i + if to_pop is not None: + self.flags.pop(to_pop) + self.flags.insert(0, flag) + + def addFlag(self, status, url, comment, username="zuul"): + flag_uid = "%s-%s-%s" % (username, self.number, self.project) flag = { - "username": username, + "username": "Zuul CI", + "user": { + "name": username + }, + "uid": flag_uid[:32], "comment": comment, "status": status, "url": url } - self.flags.insert(0, flag) + self.insertFlag(flag) self._updateTimeStamp() def editInitialComment(self, initial_comment): @@ -1405,13 +1419,18 @@ class FakePagureAPIClient(pagureconnection.PagureAPIClient): pr.is_merged = True return {}, 200, "", "POST" + match = re.match(r'.+/api/0/-/whoami$', url) + if match: + return {"username": "zuul"}, 200, "", "POST" + if not params: return self.gen_error("POST") match = re.match(r'.+/api/0/(.+)/pull-request/(\d+)/flag$', url) if match: pr = self._get_pr(match) - pr.flags.insert(0, params) + params['user'] = {"name": "zuul"} + pr.insertFlag(params) match = re.match(r'.+/api/0/(.+)/pull-request/(\d+)/comment$', url) if match: @@ -1438,9 +1457,12 @@ class FakePagureConnection(pagureconnection.PagureConnection): self.cloneurl = self.upstream_root def get_project_api_client(self, project): - return FakePagureAPIClient( + client = FakePagureAPIClient( self.baseurl, None, project, pull_requests_db=self.pull_requests) + if not self.username: + self.set_my_username(client) + return client def get_project_webhook_token(self, project): return 'fake_webhook_token-%s' % project @@ -2663,8 +2685,11 @@ class RecordingExecutorServer(zuul.executor.server.ExecutorServer): """ builds = self.running_builds[:] - self.log.debug("Releasing build %s (%s)" % (regex, - len(self.running_builds))) + if len(builds) == 0: + self.log.debug('No running builds to release') + return + + self.log.debug("Releasing build %s (%s)" % (regex, len(builds))) for build in builds: if not regex or re.match(regex, build.name): self.log.debug("Releasing build %s" % @@ -2674,7 +2699,7 @@ class RecordingExecutorServer(zuul.executor.server.ExecutorServer): self.log.debug("Not releasing build %s" % (build.parameters['zuul']['build'])) self.log.debug("Done releasing builds %s (%s)" % - (regex, len(self.running_builds))) + (regex, len(builds))) def executeJob(self, job): build = FakeBuild(self, job) @@ -4376,7 +4401,9 @@ class ZuulTestCase(BaseTestCase): def waitUntilSettled(self, msg="", matcher=None) -> None: self.log.debug("Waiting until settled... (%s)", msg) start = time.time() + i = 0 while True: + i = i + 1 if time.time() - start > self.wait_timeout: self.log.error("Timeout waiting for Zuul to settle") self.log.error("Queue status:") @@ -4393,9 +4420,10 @@ class ZuulTestCase(BaseTestCase): self.log.error("[Sched: %s] Merge client jobs: %s" % (app.sched, app.sched.merger.jobs,)) raise Exception("Timeout waiting for Zuul to settle") - # Make sure no new events show up while we're checking + # Make sure no new events show up while we're checking self.executor_server.lock.acquire() + # have all build states propogated to zuul? if self.__haveAllBuildsReported(matcher): # Join ensures that the queue is empty _and_ events have been @@ -4416,7 +4444,8 @@ class ZuulTestCase(BaseTestCase): self.scheds.execute( lambda app: app.sched.run_handler_lock.release()) self.executor_server.lock.release() - self.log.debug("...settled. (%s)", msg) + self.log.debug("...settled after %.3f ms / %s loops (%s)", + time.time() - start, i, msg) self.logState() return self.scheds.execute( @@ -4665,6 +4694,10 @@ class ZuulTestCase(BaseTestCase): completed. """ + if not self.history: + self.log.debug("Build history: no builds ran") + return + self.log.debug("Build history:") for build in self.history: self.log.debug(build) diff --git a/tests/fixtures/config/broken-multi-tenant/git/common-config/playbooks/job.yaml b/tests/fixtures/config/broken-multi-tenant/git/common-config/playbooks/job.yaml new file mode 100644 index 000000000..f679dceae --- /dev/null +++ b/tests/fixtures/config/broken-multi-tenant/git/common-config/playbooks/job.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/broken-multi-tenant/git/common-config/zuul.yaml b/tests/fixtures/config/broken-multi-tenant/git/common-config/zuul.yaml new file mode 100644 index 000000000..406a64248 --- /dev/null +++ b/tests/fixtures/config/broken-multi-tenant/git/common-config/zuul.yaml @@ -0,0 +1,23 @@ +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + failure: + gerrit: + Verified: -1 + +- job: + name: base + parent: null + run: playbooks/job.yaml + +- project: + name: ^.* + check: + jobs: + - base diff --git a/tests/fixtures/config/broken-multi-tenant/git/org_project1/README b/tests/fixtures/config/broken-multi-tenant/git/org_project1/README new file mode 100644 index 000000000..9daeafb98 --- /dev/null +++ b/tests/fixtures/config/broken-multi-tenant/git/org_project1/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/broken-multi-tenant/git/org_project2/README b/tests/fixtures/config/broken-multi-tenant/git/org_project2/README new file mode 100644 index 000000000..9daeafb98 --- /dev/null +++ b/tests/fixtures/config/broken-multi-tenant/git/org_project2/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/broken-multi-tenant/git/org_project2/zuul.yaml b/tests/fixtures/config/broken-multi-tenant/git/org_project2/zuul.yaml new file mode 100644 index 000000000..40f68b640 --- /dev/null +++ b/tests/fixtures/config/broken-multi-tenant/git/org_project2/zuul.yaml @@ -0,0 +1,3 @@ +- job: + name: child-job + parent: parent-job diff --git a/tests/fixtures/config/broken-multi-tenant/git/org_project3/README b/tests/fixtures/config/broken-multi-tenant/git/org_project3/README new file mode 100644 index 000000000..9daeafb98 --- /dev/null +++ b/tests/fixtures/config/broken-multi-tenant/git/org_project3/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/broken-multi-tenant/git/org_project3/zuul.yaml b/tests/fixtures/config/broken-multi-tenant/git/org_project3/zuul.yaml new file mode 100644 index 000000000..beef1faa0 --- /dev/null +++ b/tests/fixtures/config/broken-multi-tenant/git/org_project3/zuul.yaml @@ -0,0 +1,2 @@ +- job: + name: parent-job diff --git a/tests/fixtures/config/broken-multi-tenant/main.yaml b/tests/fixtures/config/broken-multi-tenant/main.yaml new file mode 100644 index 000000000..053056e9f --- /dev/null +++ b/tests/fixtures/config/broken-multi-tenant/main.yaml @@ -0,0 +1,19 @@ +- tenant: + name: tenant-one + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project2 + - org/project3 + +- tenant: + name: tenant-two + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project1 + - org/project2 diff --git a/tests/fixtures/layouts/requirements-github.yaml b/tests/fixtures/layouts/requirements-github.yaml index f5fa0f5de..9e376b8b6 100644 --- a/tests/fixtures/layouts/requirements-github.yaml +++ b/tests/fixtures/layouts/requirements-github.yaml @@ -89,6 +89,25 @@ check: success - pipeline: + name: require_check_run + manager: independent + require: + github: + status: + # Github does not differentiate between status and check run + # in case of branch protection and required status checks. + - check-run:tenant-one/check:success + trigger: + github: + - event: pull_request + action: comment + comment: trigger me + success: + github: + check: success + + +- pipeline: name: trigger manager: independent trigger: @@ -367,6 +386,10 @@ name: project15-check-run run: playbooks/project15-check-run.yaml +- job: + name: project16-require-check-run + run: playbooks/project16-require-check-run.yaml + - project: name: org/project1 pipeline: @@ -465,3 +488,9 @@ trigger_check_run: jobs: - project15-check-run + +- project: + name: org/project16 + require_check_run: + jobs: + - project16-require-check-run diff --git a/tests/fixtures/layouts/requirements-pagure.yaml b/tests/fixtures/layouts/requirements-pagure.yaml index 998fa7fdb..4a95d8f08 100644 --- a/tests/fixtures/layouts/requirements-pagure.yaml +++ b/tests/fixtures/layouts/requirements-pagure.yaml @@ -67,6 +67,18 @@ status: 'success' - pipeline: + name: require-flag + manager: independent + require: + pagure: + status: success + trigger: + pagure: + - event: pg_pull_request + action: status + status: success + +- pipeline: name: require-trigger-pg-closed-merged precedence: high manager: independent @@ -125,4 +137,10 @@ name: org/project6 require-trigger-pg-closed-merged: jobs: - - project-test
\ No newline at end of file + - project-test + +- project: + name: org/project7 + require-flag: + jobs: + - project-test diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py index 461df55da..e542f80f9 100644 --- a/tests/unit/test_github_requirements.py +++ b/tests/unit/test_github_requirements.py @@ -14,7 +14,7 @@ import time -from tests.base import ZuulTestCase, simple_layout +from tests.base import ZuulGithubAppTestCase, ZuulTestCase, simple_layout class TestGithubRequirements(ZuulTestCase): @@ -588,3 +588,46 @@ class TestGithubRequirements(ZuulTestCase): self.waitUntilSettled() self.assertEqual(len(self.history), 2) self.assertEqual(self.history[1].name, 'project12-status') + + +class TestGithubAppRequirements(ZuulGithubAppTestCase): + """Test pipeline and trigger requirements with app authentication""" + config_file = 'zuul-github-driver.conf' + + @simple_layout("layouts/requirements-github.yaml", driver="github") + def test_pipeline_require_check_run(self): + "Test pipeline requirement: status (reported via a check run)" + project = "org/project16" + github = self.fake_github.getGithubClient() + repo = github.repo_from_project(project) + + A = self.fake_github.openFakePullRequest(project, "master", "A") + # A comment event that we will keep submitting to trigger + comment = A.getCommentAddedEvent("trigger me") + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + + # No status from zuul, so nothing should be enqueued + self.assertEqual(len(self.history), 0) + + # An error check run should also not cause it to be enqueued + repo.create_check_run( + A.head_sha, + "tenant-one/check", + conclusion="failure", + app="check-run", + ) + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + self.assertEqual(len(self.history), 0) + + # A success check run goes in, ready to be enqueued + repo.create_check_run( + A.head_sha, + "tenant-one/check", + conclusion="success", + app="check-run", + ) + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + self.assertEqual(len(self.history), 1) diff --git a/tests/unit/test_pagure_driver.py b/tests/unit/test_pagure_driver.py index 04daee45a..0393a13d0 100644 --- a/tests/unit/test_pagure_driver.py +++ b/tests/unit/test_pagure_driver.py @@ -61,9 +61,8 @@ class TestPagureDriver(ZuulTestCase): self.assertThat( A.comments[1]['comment'], MatchesRegex(r'.*\[project-test2 \]\(.*\).*', re.DOTALL)) - self.assertEqual(2, len(A.flags)) + self.assertEqual(1, len(A.flags)) self.assertEqual('success', A.flags[0]['status']) - self.assertEqual('pending', A.flags[1]['status']) @simple_layout('layouts/basic-pagure.yaml', driver='pagure') def test_pull_request_updated(self): @@ -493,6 +492,31 @@ class TestPagureDriver(ZuulTestCase): self.assertEqual(1, len(self.history)) @simple_layout('layouts/requirements-pagure.yaml', driver='pagure') + def test_flag_require(self): + + A = self.fake_pagure.openFakePullRequest( + 'org/project7', 'master', 'A') + + # CI status from other CIs must not be handled + self.fake_pagure.emitEvent( + A.getPullRequestStatusSetEvent("success", username="notzuul")) + self.waitUntilSettled() + self.assertEqual(0, len(self.history)) + self.assertEqual(1, len(A.flags)) + + self.fake_pagure.emitEvent( + A.getPullRequestStatusSetEvent("failure")) + self.waitUntilSettled() + self.assertEqual(0, len(self.history)) + self.assertEqual(2, len(A.flags)) + + self.fake_pagure.emitEvent( + A.getPullRequestStatusSetEvent("success")) + self.waitUntilSettled() + self.assertEqual(1, len(self.history)) + self.assertEqual(2, len(A.flags)) + + @simple_layout('layouts/requirements-pagure.yaml', driver='pagure') def test_pull_request_closed(self): A = self.fake_pagure.openFakePullRequest( diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index 7753b9633..a80675544 100644 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -3358,6 +3358,80 @@ class TestBrokenConfig(ZuulTestCase): "A should have failed the check pipeline") +class TestBrokenMultiTenantConfig(ZuulTestCase): + # Test we can deal with a broken multi-tenant config + + tenant_config_file = 'config/broken-multi-tenant/main.yaml' + + def test_loading_errors(self): + # This regression test came about when we discovered the following: + + # * We cache configuration objects if they load without error + # in their first tenant; that means that they can show up as + # errors in later tenants, but as long as those other + # tenants aren't proposing changes to that repo (which is + # unlikely in this situation; this usually arises if the + # tenant just wants to use some foreign jobs), users won't + # be blocked by the error. + # + # * If a merge job for a dynamic config change arrives out of + # order, we will build the new configuration and if there + # are errors, we will compare it to the previous + # configuration to determine if they are relevant, but that + # caused an error since the previous layout had not been + # calculated yet. It's pretty hard to end up with + # irrelevant errors except by virtue of the first point + # above, which is why this test relies on a second tenant. + + # This test has two tenants. The first loads project2, and + # project3 without errors and all config objects are cached. + # The second tenant loads only project1 and project2. + # Project2 references a job that is defined in project3, so + # the tenant loads with an error, but proceeds. + + # Don't run any merge jobs, so we can run them out of order. + self.gearman_server.hold_merge_jobs_in_queue = True + + # Create a first change which modifies the config (and + # therefore will require a merge job). + in_repo_conf = textwrap.dedent( + """ + - job: {'name': 'foo'} + """) + file_dict = {'.zuul.yaml': in_repo_conf} + A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A', + files=file_dict) + + # Create a second change which also modifies the config. + B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B', + files=file_dict) + B.setDependsOn(A, 1) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + # There should be a merge job for each change. + self.assertEqual(len(self.scheds.first.sched.merger.jobs), 2) + + jobs = [job for job in self.gearman_server.getQueue() + if job.name.startswith(b'merger:')] + # Release the second merge job. + jobs[-1].waiting = False + self.gearman_server.wakeConnections() + self.waitUntilSettled() + + # At this point we should still be waiting on the first + # change's merge job. + self.assertHistory([]) + + # Proceed. + self.gearman_server.hold_merge_jobs_in_queue = False + self.gearman_server.release() + self.waitUntilSettled() + self.assertHistory([ + dict(name='base', result='SUCCESS', changes='1,1 2,1'), + ]) + + class TestProjectKeys(ZuulTestCase): # Test that we can generate project keys diff --git a/tools/pip.sh b/tools/pip.sh index 469d119eb..7db784690 100755 --- a/tools/pip.sh +++ b/tools/pip.sh @@ -23,7 +23,15 @@ then pip install nodeenv # Initialize nodeenv and tell it to re-use the currently active virtualenv # TODO(jeblair): remove node version pin. upath 1.0.4 objects to node >9. - nodeenv --python-virtualenv -n 10.16.0 + attempts=0 + until nodeenv --python-virtualenv -n 10.16.0; do + ((attempts++)) + if [[ $attempts > 2 ]] + then + echo "Failed creating nodeenv" + exit 1 + fi + done # Use -g because inside of the virtualenv '-g' means 'install into the' # virtualenv - as opposed to installing into the local node_modules. # Avoid writing a package-lock.json file since we don't use it. diff --git a/zuul/ansible/base/callback/zuul_stream.py b/zuul/ansible/base/callback/zuul_stream.py index 5e8d8cd3b..7cae6efa5 100644 --- a/zuul/ansible/base/callback/zuul_stream.py +++ b/zuul/ansible/base/callback/zuul_stream.py @@ -663,7 +663,7 @@ class CallbackModule(default.CallbackModule): host=hostname, status=status, msg=msg)) else: self._log("{host} | {status}".format( - host=hostname, status=status, msg=msg)) + host=hostname, status=status)) if result_dict: result_string = json.dumps(result_dict, indent=2, sort_keys=True) for line in result_string.split('\n'): diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 19497008d..a054d9e49 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -2085,6 +2085,17 @@ class GithubConnection(BaseConnection): statuses.append("%s:%s:%s" % stuple) seen.append("%s:%s" % (stuple[0], stuple[1])) + # Although Github differentiates commit statuses and commit checks via + # their respective APIs, the branch protection the status section + # (below the comments of a PR) do not differentiate between both. Thus, + # to mimic this behaviour also in Zuul, a required_status in the + # pipeline config could map to either a status or a check. + for check in self.getCommitChecks(project.name, sha, event): + ctuple = _check_as_tuple(check) + if "{}:{}".format(ctuple[0], ctuple[1]) not in seen: + statuses.append("{}:{}:{}".format(*ctuple)) + seen.append("{}:{}".format(ctuple[0], ctuple[1])) + return statuses def getWebController(self, zuul_web): diff --git a/zuul/driver/pagure/pagureconnection.py b/zuul/driver/pagure/pagureconnection.py index 19a68ef0f..becc803c8 100644 --- a/zuul/driver/pagure/pagureconnection.py +++ b/zuul/driver/pagure/pagureconnection.py @@ -65,7 +65,6 @@ from zuul.driver.pagure.paguremodel import PagureTriggerEvent, PullRequest # # Repository settings (to be checked): # - Minimum score to merge pull-request = 0 or -1 -# - Notify on pull-request flag # - Pull requests # - Open metadata access to all (unchecked if approval) # @@ -206,6 +205,7 @@ class PagureEventConnector(threading.Thread): 'pull-request.closed': self._event_pull_request_closed, 'pull-request.new': self._event_pull_request, 'pull-request.flag.added': self._event_flag_added, + 'pull-request.flag.updated': self._event_flag_added, 'git.receive': self._event_ref_updated, 'git.branch.creation': self._event_ref_created, 'git.branch.deletion': self._event_ref_deleted, @@ -438,6 +438,12 @@ class PagureAPIClient(): ret.status_code, ret.text)) return ret.json(), ret.status_code, ret.url, 'POST' + def whoami(self): + path = '-/whoami' + resp = self.post(self.base_url + path) + self._manage_error(*resp) + return resp[0]['username'] + def get_project_branches(self): path = '%s/git/branches' % self.project resp = self.get(self.base_url + path) @@ -456,23 +462,29 @@ class PagureAPIClient(): self._manage_error(*resp) return resp[0] - def get_pr_flags(self, number, last=False): + def get_pr_flags(self, number, owner, last=False): path = '%s/pull-request/%s/flag' % (self.project, number) resp = self.get(self.base_url + path) self._manage_error(*resp) data = resp[0] + owned_flags = [ + flag for flag in data['flags'] + if flag['user']['name'] == owner] if last: - if data['flags']: - return data['flags'][0] + if owned_flags: + return owned_flags[0] else: return {} else: - return data['flags'] + return owned_flags - def set_pr_flag(self, number, status, url, description): + def set_pr_flag( + self, number, status, url, description, app_name, username): + flag_uid = "%s-%s-%s" % (username, number, self.project) params = { - "username": "Zuul", + "username": app_name, "comment": "Jobs result is %s" % status, + "uid": flag_uid[:32], "status": status, "url": url} path = '%s/pull-request/%s/flag' % (self.project, number) @@ -494,18 +506,17 @@ class PagureAPIClient(): return resp[0] def get_webhook_token(self): - """ A project admin user's api token must be use with that endpoint + """ A project collaborator's api token must be used with that endpoint """ - path = '%s/connector' % self.project + path = '%s/webhook/token' % self.project resp = self.get(self.base_url + path) self._manage_error(*resp) - return resp[0]['connector']['hook_token'] + return resp[0]['webhook']['token'] class PagureConnection(BaseConnection): driver_name = 'pagure' log = logging.getLogger("zuul.PagureConnection") - payload_path = 'payload' def __init__(self, driver, connection_name, connection_config): super(PagureConnection, self).__init__( @@ -518,6 +529,9 @@ class PagureConnection(BaseConnection): 'canonical_hostname', self.server) self.git_ssh_key = self.connection_config.get('sshkey') self.api_token = self.connection_config.get('api_token') + self.app_name = self.connection_config.get( + 'app_name', 'Zuul') + self.username = None self.baseurl = self.connection_config.get( 'baseurl', 'https://%s' % self.server).rstrip('/') self.cloneurl = self.connection_config.get( @@ -562,9 +576,17 @@ class PagureConnection(BaseConnection): def eventDone(self): self.event_queue.task_done() + def set_my_username(self, client): + self.log.debug("Fetching my username ...") + self.username = client.whoami() + self.log.debug("My username is %s" % self.username) + def get_project_api_client(self, project): self.log.debug("Building project %s api_client" % project) - return PagureAPIClient(self.baseurl, self.api_token, project) + client = PagureAPIClient(self.baseurl, self.api_token, project) + if not self.username: + self.set_my_username(client) + return client def get_project_webhook_token(self, project, force_refresh=False): token = self.webhook_tokens.get(project) @@ -696,7 +718,7 @@ class PagureConnection(BaseConnection): def _hasRequiredStatusChecks(self, change): pagure = self.get_project_api_client(change.project.name) - flag = pagure.get_pr_flags(change.number, last=True) + flag = pagure.get_pr_flags(change.number, self.username, last=True) return True if flag.get('status', '') == 'success' else False def canMerge(self, change, allow_needs, event=None): @@ -804,14 +826,15 @@ class PagureConnection(BaseConnection): def setCommitStatus(self, project, number, state, url='', description='', context=''): pagure = self.get_project_api_client(project) - pagure.set_pr_flag(number, state, url, description) + pagure.set_pr_flag( + number, state, url, description, self.app_name, self.username) self.log.info("Set pull-request CI flag status : %s" % description) # Wait for 1 second as flag timestamp is by second time.sleep(1) def getCommitStatus(self, project, number): pagure = self.get_project_api_client(project) - flag = pagure.get_pr_flags(number, last=True) + flag = pagure.get_pr_flags(number, self.username, last=True) self.log.info( "Got pull-request CI status for PR %s on %s status: %s" % ( number, project, flag.get('status'))) diff --git a/zuul/executor/server.py b/zuul/executor/server.py index 4f73e20de..5cb66f759 100644 --- a/zuul/executor/server.py +++ b/zuul/executor/server.py @@ -2215,7 +2215,7 @@ class AnsibleJob(object): stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, - preexec_fn=os.setsid, + start_new_session=True, env=env_copy, ) @@ -2702,7 +2702,7 @@ class ExecutorServer(BaseMergeServer): self.command_thread.daemon = True self.command_thread.start() - self.log.debug("Starting workers") + self.log.debug("Starting %s update workers" % self.update_workers) for i in range(self.update_workers): update_thread = threading.Thread(target=self._updateLoop, name='update') diff --git a/zuul/lib/ansible-config.conf b/zuul/lib/ansible-config.conf index c311c52ca..e8920cb48 100644 --- a/zuul/lib/ansible-config.conf +++ b/zuul/lib/ansible-config.conf @@ -8,6 +8,7 @@ deprecated = true requirements = ansible>=2.6,<2.7 [2.7] +deprecated = true requirements = ansible>=2.7,<2.8 [2.8] diff --git a/zuul/lib/gearworker.py b/zuul/lib/gearworker.py index 1bffbd4de..1e1433692 100644 --- a/zuul/lib/gearworker.py +++ b/zuul/lib/gearworker.py @@ -51,18 +51,18 @@ class ZuulGearWorker: self.ssl_cert, self.ssl_ca, keepalive=True, tcp_keepidle=60, tcp_keepintvl=30, tcp_keepcnt=5) - self.log.debug('Waiting for server') + self.log.debug('Waiting for gearman') self.gearman.waitForServer() self.register() self.thread.start() def register(self): - self.log.debug('Registering jobs') + self.log.debug('Registering %s jobs' % len(self.jobs)) for job in self.jobs: self.gearman.registerFunction(job) def unregister(self): - self.log.debug('Unregistering jobs') + self.log.debug('Unregistering all jobs (%s)' % len(self.jobs)) for job in self.jobs: self.gearman.unRegisterFunction(job) diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 469d2c1b3..ee0f2c241 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -563,19 +563,6 @@ class PipelineManager(metaclass=ABCMeta): zuul_event_id=None) untrusted_errors = len(untrusted_layout.loading_errors) > 0 - # TODO (jeblair): remove this section of extra verbose - # debug logging when we have resolved the loading_errors - # bug. - log.debug("Dynamic layout: trusted errors: %s layout: %s", - trusted_errors, trusted_layout) - if trusted_layout: - for err in trusted_layout.loading_errors.errors[:10]: - log.debug(err.error) - log.debug("Dynamic layout: untrusted errors: %s layout: %s", - untrusted_errors, untrusted_layout) - if untrusted_layout: - for err in untrusted_layout.loading_errors.errors[:10]: - log.debug(err.error) # Configuration state handling switchboard. Intentionally verbose # and repetetive to be exceptionally clear that we handle all # possible cases correctly. Note we never return trusted_layout @@ -666,6 +653,9 @@ class PipelineManager(metaclass=ABCMeta): def getLayout(self, item): if item.item_ahead: fallback_layout = item.item_ahead.layout + if fallback_layout is None: + # We're probably waiting on a merge job for the item ahead. + return None else: fallback_layout = item.pipeline.tenant.layout if not item.change.updatesConfig(item.pipeline.tenant): diff --git a/zuul/merger/merger.py b/zuul/merger/merger.py index 7e1188730..4d6e6f31c 100644 --- a/zuul/merger/merger.py +++ b/zuul/merger/merger.py @@ -287,20 +287,20 @@ class Repo(object): repo = Repo._createRepoObject(local_path, env) origin = repo.remotes.origin - # Reset the working directory to the default remote branch. + # Detach HEAD so we can work with references without interfering + # with any active branch. Any remote ref will do as long as it can + # be dereferenced to an existing commit. for ref in origin.refs: - if ref.remote_head != "HEAD": - continue - # Use the ref the remote HEAD is pointing to - head_ref = ref.ref - head = head_ref.remote_head - repo.create_head(head, head_ref, force=True) - if log: - log.debug("Reset to %s", head) - else: - messages.append("Reset to %s" % head) - repo.head.reference = head - break + try: + repo.head.reference = ref.commit + break + except Exception: + if log: + log.debug("Unable to detach HEAD to %s", ref) + else: + messages.append("Unable to detach HEAD to %s" % ref) + else: + raise Exception("Couldn't detach HEAD to any existing commit") # Delete local heads that no longer exist on the remote end remote_heads = {r.remote_head for r in origin.refs} @@ -340,15 +340,6 @@ class Repo(object): for message in messages: log.debug(message) - def prune(self, zuul_event_id=None): - log = get_annotated_logger(self.log, zuul_event_id) - repo = self.createRepoObject(zuul_event_id) - origin = repo.remotes.origin - stale_refs = origin.stale_refs - if stale_refs: - log.debug("Pruning stale refs: %s", stale_refs) - git.refs.RemoteReference.delete(repo, force=True, *stale_refs) - def getBranchHead(self, branch, zuul_event_id=None): repo = self.createRepoObject(zuul_event_id) branch_head = repo.heads[branch] |