diff --git a/doc/source/drivers/github.rst b/doc/source/drivers/github.rst index 500adb43c4..8ebd9459be 100644 --- a/doc/source/drivers/github.rst +++ b/doc/source/drivers/github.rst @@ -292,6 +292,13 @@ the following options. .. attr:: event :required: + .. warning:: While it is currently possible to list more than + one event in a single trigger, that is deprecated + and support will be removed in a future version. + Instead, specify a single event type per trigger, + and list multiple triggers as necessary to cover + all intended events. + The event from github. Supported events are: .. value:: pull_request @@ -359,7 +366,17 @@ the following options. .. value:: requested - A check run is requested. + .. warning:: This is deprecated and will be removed in a + future version. Use + :value:`pipeline.trigger..action.rerequested` instead. + + Deprecated alias for :value:`pipeline.trigger..action.rerequested`. + + .. value:: rerequested + + A check run is rerequested. .. value:: completed @@ -400,7 +417,7 @@ the following options. .. attr:: status - This is used for ``pull-request`` and ``status`` actions. It + This is used for ``pull_request`` and ``status`` actions. It accepts a list of strings each of which matches the user setting the status, the status context, and the status itself in the format of ``user:context:status``. For example, diff --git a/releasenotes/notes/github-trigger-deprecations-067ba9feab4c0a76.yaml b/releasenotes/notes/github-trigger-deprecations-067ba9feab4c0a76.yaml new file mode 100644 index 0000000000..b87e2df585 --- /dev/null +++ b/releasenotes/notes/github-trigger-deprecations-067ba9feab4c0a76.yaml @@ -0,0 +1,15 @@ +deprecations: + - | + Specifying :attr:`pipeline.trigger..event` as a + list is deprecated. Update any instances to specify multiple + triggers each with a single associated event instead. + - | + The use of :value:`pipeline.trigger..action.requested` has been deprecated for some time + already. Its use will now produce a configuration syntax warning, + and in a future version of Zuul, an error. + - | + The use of :attr:`pipeline.trigger..require-status` + has been deprecated for some already. Its use will now produce a + configuration syntax warning, and in a future version of Zuul, an + error. diff --git a/tests/fixtures/layouts/github-merge-mode.yaml b/tests/fixtures/layouts/github-merge-mode.yaml index 08a76cae28..467c316789 100644 --- a/tests/fixtures/layouts/github-merge-mode.yaml +++ b/tests/fixtures/layouts/github-merge-mode.yaml @@ -5,7 +5,7 @@ github: - event: pull_request_review action: submitted - state: approve + state: approved - event: pull_request action: - opened diff --git a/tests/fixtures/layouts/github-message-update.yaml b/tests/fixtures/layouts/github-message-update.yaml index ec6551677b..8a22fa7f20 100644 --- a/tests/fixtures/layouts/github-message-update.yaml +++ b/tests/fixtures/layouts/github-message-update.yaml @@ -5,7 +5,7 @@ github: - event: pull_request_review action: submitted - state: approve + state: approved start: github: {} success: diff --git a/tests/fixtures/layouts/github-schema.yaml b/tests/fixtures/layouts/github-schema.yaml new file mode 100644 index 0000000000..75d56c304a --- /dev/null +++ b/tests/fixtures/layouts/github-schema.yaml @@ -0,0 +1,43 @@ +- pipeline: + name: check + manager: independent + trigger: + github: + # Valid + - event: pull_request + action: comment + comment: test me + # Valid + - event: pull_request + action: + - opened + - closed + # "extra keys not allowed @ data['check']" + - event: pull_request + action: + - opened + - closed + branch: ^master$ + check: foo + # "extra keys not allowed @ data['branch']" + - event: check_run + branch: ^master$ + # "as a list is deprecated" + - event: + - pull_request + - check_run + # "'require-status' trigger attribute" + # "extra keys not allowed @ data['require-status']" + - event: check_run + require-status: foo + # "'unlabel' trigger attribute" + # "extra keys not allowed @ data['unlabel']" + - event: check_run + unlabel: foo + # "Use 'rerequested' instead" + - event: check_run + action: requested + success: + github: {} + failure: + github: {} diff --git a/tests/fixtures/layouts/pcre-deprecation-github.yaml b/tests/fixtures/layouts/pcre-deprecation-github.yaml index 3d39b77661..6347d47afe 100644 --- a/tests/fixtures/layouts/pcre-deprecation-github.yaml +++ b/tests/fixtures/layouts/pcre-deprecation-github.yaml @@ -3,9 +3,7 @@ manager: independent trigger: github: - - event: pull_request_review - action: submitted - state: approve + - event: push ref: '(?!invalid)' start: github: {} diff --git a/tests/fixtures/layouts/reviews-github.yaml b/tests/fixtures/layouts/reviews-github.yaml index d475379445..ba85becaae 100644 --- a/tests/fixtures/layouts/reviews-github.yaml +++ b/tests/fixtures/layouts/reviews-github.yaml @@ -5,7 +5,7 @@ github: - event: pull_request_review action: submitted - state: approve + state: approved success: github: label: diff --git a/tests/unit/test_github_crd.py b/tests/unit/test_github_crd.py index 4d9aad0ac1..d35cb73b75 100644 --- a/tests/unit/test_github_crd.py +++ b/tests/unit/test_github_crd.py @@ -195,7 +195,7 @@ class TestGithubCrossRepoDeps(ZuulTestCase): self.executor_server.hold_jobs_in_build = True # Enqueue A,B,C - self.fake_github.emitEvent(C.getReviewAddedEvent('approve')) + self.fake_github.emitEvent(C.getReviewAddedEvent('approved')) self.waitUntilSettled() self.assertEqual(len(self.builds), 1) diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index 97d497835e..7c5e501b7a 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -375,7 +375,7 @@ class TestGithubDriver(ZuulTestCase): @simple_layout('layouts/reviews-github.yaml', driver='github') def test_reviews(self): A = self.fake_github.openFakePullRequest('org/project', 'master', 'A') - self.fake_github.emitEvent(A.getReviewAddedEvent('approve')) + self.fake_github.emitEvent(A.getReviewAddedEvent('approved')) self.waitUntilSettled() self.assertEqual(1, len(self.history)) self.assertEqual('project-reviews', self.history[0].name) @@ -383,7 +383,7 @@ class TestGithubDriver(ZuulTestCase): # test_review_unmatched_event B = self.fake_github.openFakePullRequest('org/project', 'master', 'B') - self.fake_github.emitEvent(B.getReviewAddedEvent('comment')) + self.fake_github.emitEvent(B.getReviewAddedEvent('commented')) self.waitUntilSettled() self.assertEqual(1, len(self.history)) @@ -2790,3 +2790,38 @@ class TestGithubDefaultBranch(ZuulTestCase): self.assertEqual('foobar', md.default_branch) new_layout = layout.uuid self.assertNotEqual(new_layout, prev_layout) + + +class TestGithubSchemaWarnings(ZuulTestCase): + config_file = 'zuul-github-driver.conf' + + @simple_layout('layouts/github-schema.yaml', driver='github') + def test_broken_config_on_startup_warnings(self): + tenant = self.scheds.first.sched.abide.tenants.get('tenant-one') + self.assertEquals( + len(tenant.layout.loading_errors), 8, + "An error should have been stored") + self.assertIn( + "extra keys not allowed @ data['check']", + str(tenant.layout.loading_errors[0].error)) + self.assertIn( + "extra keys not allowed @ data['branch']", + str(tenant.layout.loading_errors[1].error)) + self.assertIn( + "as a list is deprecated", + str(tenant.layout.loading_errors[2].error)) + self.assertIn( + "'require-status' trigger attribute", + str(tenant.layout.loading_errors[3].error)) + self.assertIn( + "extra keys not allowed @ data['require-status']", + str(tenant.layout.loading_errors[4].error)) + self.assertIn( + "'unlabel' trigger attribute", + str(tenant.layout.loading_errors[5].error)) + self.assertIn( + "extra keys not allowed @ data['unlabel']", + str(tenant.layout.loading_errors[6].error)) + self.assertIn( + "Use 'rerequested' instead", + str(tenant.layout.loading_errors[7].error)) diff --git a/zuul/driver/github/githubtrigger.py b/zuul/driver/github/githubtrigger.py index 37c5ada2bf..90641faa40 100644 --- a/zuul/driver/github/githubtrigger.py +++ b/zuul/driver/github/githubtrigger.py @@ -14,34 +14,78 @@ # under the License. import logging -import voluptuous as v +import voluptuous as vs from zuul.trigger import BaseTrigger from zuul.driver.github.githubmodel import GithubEventFilter from zuul.driver.github import githubsource from zuul.driver.util import scalar_or_list, to_list, make_regex, ZUUL_REGEX +from zuul.configloader import DeprecationWarning + + +class GithubUnlabelDeprecation(DeprecationWarning): + zuul_error_name = 'Github unlabel Deprecation' + zuul_error_message = """The 'unlabel' trigger attribute +is deprecated. Use 'label' instead.""" + + +class GithubEventListDeprecation(DeprecationWarning): + zuul_error_name = 'Github event list Deprecation' + zuul_error_message = """Specifying the 'event' trigger attribute +as a list is deprecated. Use a single item instead.""" + + +class GithubRequireStatusDeprecation(DeprecationWarning): + zuul_error_name = 'Github require-status Deprecation' + zuul_error_message = """The 'require-status' trigger attribute +is deprecated. Use 'require' instead.""" + + +class GithubRequestedActionDeprecation(DeprecationWarning): + zuul_error_name = 'Github requested action Deprecation' + zuul_error_message = """The 'requested' value for the 'action' +trigger attribute is deprecated. Use 'rerequested' instead.""" + + +class GithubTriggerConfigurationWarning(DeprecationWarning): + zuul_error_name = 'Github Trigger Configuration Warning' class GithubTrigger(BaseTrigger): name = 'github' log = logging.getLogger("zuul.trigger.GithubTrigger") - def __init__(self, driver, connection, config=None): - - # This is a compatibility layer to map the action 'requested' back - # to the original action 'rerequested'. - # TODO: Remove after zuul 5.0 - for item in config: - if item.get('action') == 'requested': - item['action'] = 'rerequested' - - super().__init__(driver, connection, config=config) - def getEventFilters(self, connection_name, trigger_config, parse_context): efilters = [] pcontext = parse_context + new_schema = getNewSchema() + for trigger in to_list(trigger_config): + # Deprecated in 8.3.0 but warning added in 10.0 + if 'require-status' in trigger: + with pcontext.confAttr(trigger, 'require-status'): + pcontext.accumulator.addError( + GithubRequireStatusDeprecation()) + # Deprecated with warning in 10.0 + if 'unlabel' in trigger: + with pcontext.confAttr(trigger, 'unlabel'): + pcontext.accumulator.addError( + GithubUnlabelDeprecation()) + # Deprecated with warning in 10.0 + if isinstance(trigger.get('event', None), list): + with pcontext.confAttr(trigger, 'event'): + pcontext.accumulator.addError( + GithubEventListDeprecation()) + + try: + new_schema(trigger) + except vs.Invalid as e: + pcontext.accumulator.addError( + GithubTriggerConfigurationWarning( + "The trigger configuration as supplied " + f"has an error: {e}")) + with pcontext.confAttr(trigger, 'event') as attr: types = [make_regex(x, pcontext) for x in to_list(attr)] @@ -55,6 +99,18 @@ class GithubTrigger(BaseTrigger): comments = [make_regex(x, pcontext) for x in to_list(attr)] + # This is a compatibility layer to map the action 'requested' back + # to the original action 'rerequested'. + # TODO: Remove after zuul 5.0 + # Note the original backwards compat handling for this did + # not allow 'requested' as a list, so we don't do that + # here either. + if trigger.get('action') == 'requested': + trigger['action'] = 'rerequested' + with pcontext.confAttr(trigger, 'action'): + pcontext.accumulator.addError( + GithubRequestedActionDeprecation()) + f = GithubEventFilter( connection_name=connection_name, trigger=self, @@ -80,17 +136,119 @@ class GithubTrigger(BaseTrigger): pass +def getNewSchema(): + # For now, this is only used to raise deprecation errors if the + # syntax does not match. This was added in 10.0. When we're + # ready to enforce this, rename this method getSchema. + base_schema = vs.Schema({ + vs.Required('event'): vs.Any('pull_request', + 'pull_request_review', + 'push', + 'check_run'), + 'require': githubsource.getRequireSchema(), + 'reject': githubsource.getRejectSchema(), + }) + + # Pull request + pull_request_base_schema = base_schema.extend({ + vs.Required('event'): 'pull_request', + 'branch': scalar_or_list(vs.Any(ZUUL_REGEX, str)), + }) + + pull_request_default_schema = pull_request_base_schema.extend({ + 'action': scalar_or_list(vs.Any( + 'opened', 'changed', 'closed', 'reopened')), + }) + + pull_request_comment_schema = pull_request_base_schema.extend({ + vs.Required('action'): 'comment', + 'comment': scalar_or_list(vs.Any(ZUUL_REGEX, str)), + }) + + pull_request_labeled_schema = pull_request_base_schema.extend({ + vs.Required('action'): scalar_or_list(vs.Any('labeled', 'unlabeled')), + 'label': scalar_or_list(str), + }) + + pull_request_status_schema = pull_request_base_schema.extend({ + vs.Required('action'): 'status', + 'status': scalar_or_list(str), + }) + + # Pull request review + pull_request_review_base_schema = base_schema.extend({ + vs.Required('event'): 'pull_request_review', + 'branch': scalar_or_list(vs.Any(ZUUL_REGEX, str)), + }) + + pull_request_review_schema = pull_request_review_base_schema.extend({ + 'action': scalar_or_list(vs.Any('submitted', 'dismissed')), + 'state': scalar_or_list(vs.Any( + 'approved', 'commented', 'changes_requested', + 'dismissed', 'pending')), + }) + + # Push + push_schema = base_schema.extend({ + vs.Required('event'): 'push', + 'ref': scalar_or_list(vs.Any(ZUUL_REGEX, str)), + }) + + # Check run + check_run_schema = base_schema.extend({ + vs.Required('event'): 'check_run', + 'action': scalar_or_list(vs.Any('requested', 'completed')), + 'check': scalar_or_list(str), + }) + + init_schema = base_schema.extend({}, extra=vs.ALLOW_EXTRA) + + def validate(data): + # This method could be a simple vs.Any() across all the + # possibilities, but sometimes the error messages are less + # intuitive (e.g., indicating that the action is wrong rather + # than that a user has added an attribute that is incompatible + # with the action). Instead, we assume the user got the event + # and/or action right first, then apply the appropriate schema + # for what they selected. + + event = data.get('event') + # TODO: run this unconditionally after + # GithubEventListDeprecation is complete + if isinstance(event, str): + # First, make sure the common items are correct. + init_schema(data) + action = data.get('action') + if event == 'pull_request': + if action == 'comment': + pull_request_comment_schema(data) + elif action == 'labeled': + pull_request_labeled_schema(data) + elif action == 'status': + pull_request_status_schema(data) + else: + pull_request_default_schema(data) + elif event == 'pull_request_review': + pull_request_review_schema(data) + elif event == 'push': + push_schema(data) + elif event == 'check_run': + check_run_schema(data) + + return validate + + def getSchema(): - github_trigger = { - v.Required('event'): - scalar_or_list(v.Any('pull_request', - 'pull_request_review', - 'push', - 'check_run')), + old_schema = vs.Schema({ + vs.Required('event'): + scalar_or_list(vs.Any('pull_request', + 'pull_request_review', + 'push', + 'check_run')), 'action': scalar_or_list(str), - 'branch': scalar_or_list(v.Any(ZUUL_REGEX, str)), - 'ref': scalar_or_list(v.Any(ZUUL_REGEX, str)), - 'comment': scalar_or_list(v.Any(ZUUL_REGEX, str)), + 'branch': scalar_or_list(vs.Any(ZUUL_REGEX, str)), + 'ref': scalar_or_list(vs.Any(ZUUL_REGEX, str)), + 'comment': scalar_or_list(vs.Any(ZUUL_REGEX, str)), 'label': scalar_or_list(str), 'unlabel': scalar_or_list(str), 'state': scalar_or_list(str), @@ -99,6 +257,6 @@ def getSchema(): 'reject': githubsource.getRejectSchema(), 'status': scalar_or_list(str), 'check': scalar_or_list(str), - } + }) - return github_trigger + return old_schema