From 171d4c56b1f1ee1f712b91f4ee466a9a37f9d298 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Fri, 16 Feb 2024 13:32:44 -0800 Subject: [PATCH] Add some github configuration deprecations The "event" trigger attribute can currently be a list. Technically, it is possible to construct a trigger with an event list, such as: trigger: github: - event: - pull_request - pull_request_review branch: master Which would trigger on any pull_request or pull_request_review event on the master branch. However in practice users typically have much more narrow event selections, such as only triggering on pull_request events with the opened action, or a pull_request event with a certain comment. It is not possible to construct that example with a single trigger; the following is invalid: trigger: github: - event: - pull_request - pull_request_review actions: - opened - commented branch: master comment: recheck That will pass syntax validation but would only fire on a recheck comment; it would never fire on a PR opened event because that event won't have a comment. To help users avoid these problems, or worse, let's limit the event specifier to a single event (of course users can add more triggers for other events). That will allow us to inform users when they use options incompatible with the event they selected. For now, we make this a deprecation so that in the future we can enforce it and improve feedback. This adds syntax validation for each of the possible event/action combinations in the case where the user has already specified a single event. This allows us to go ahead and issue warnings if users specify incompatible options. Later, all of these can become errors. Some time ago (8.3.0) we deprecated the require-status attribute. It is eligible for removal now, but that predated the deprecation warnings system. Since we haven't yet removed it, and we now have that system, let's add a deprecation warning for it and give users a little more time to notice that and remove it before it becomes an error. When a Github user requests that a check run start again, Github emits a "check_run" event with a "rerequested" action. In zuul < 5.0.0, we asked users to configure the check_run trigger with the "requested" action and we silently translated the "rerequested" from github to the zuul "requested". In 5.0.0, we reversed that decision in order to match our policy of passing through data from remote systems as closely as possible to enable users to match the corresponding documentation of zuul and the remote system. We deprecated "requested" and updated the examples in the documentation to say "rerequested". Unfortunately, we left the canonical documentation of the value as "requested". To correct this oversight, that documentation is updated to say "rerequested" and a configuration deprecation warning is added for uses of "requested". The "unabel" trigger attribute is undocumented and unused. Deprecate it from syntax checking here so we can gracefully remove it later. Some unit tests configs are updated since they passed validation previously but no longer do, and the actual github pull request review state constants ('approved', etc) are updated to match what github sends. Change-Id: I6bf7753d74ec0c5f19dad508c33762a7803fe805 --- doc/source/drivers/github.rst | 21 +- ...trigger-deprecations-067ba9feab4c0a76.yaml | 15 ++ tests/fixtures/layouts/github-merge-mode.yaml | 2 +- .../layouts/github-message-update.yaml | 2 +- tests/fixtures/layouts/github-schema.yaml | 43 ++++ .../layouts/pcre-deprecation-github.yaml | 4 +- tests/fixtures/layouts/reviews-github.yaml | 2 +- tests/unit/test_github_crd.py | 2 +- tests/unit/test_github_driver.py | 39 +++- zuul/driver/github/githubtrigger.py | 204 ++++++++++++++++-- 10 files changed, 300 insertions(+), 34 deletions(-) create mode 100644 releasenotes/notes/github-trigger-deprecations-067ba9feab4c0a76.yaml create mode 100644 tests/fixtures/layouts/github-schema.yaml 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