zuul
/
zuul
Code Issues Proposed changes

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
This commit is contained in:
James E. Blair 2024-02-16 13:32:44 -08:00
parent dc7c896af2
commit 171d4c56b1
10 changed files with 300 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
doc/source/drivers/github.rst
View File

@ -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.<github
source>.action.rerequested` instead.
Deprecated alias for :value:`pipeline.trigger.<github
source>.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,

15
releasenotes/notes/github-trigger-deprecations-067ba9feab4c0a76.yaml Normal file
View File

@ -0,0 +1,15 @@
deprecations:
- |
Specifying :attr:`pipeline.trigger.<github source>.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.<github
source>.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.<github source>.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.

2
tests/fixtures/layouts/github-merge-mode.yaml vendored
View File

@ -5,7 +5,7 @@
github:
- event: pull_request_review
action: submitted
state: approve
state: approved
- event: pull_request
action:
- opened

2
tests/fixtures/layouts/github-message-update.yaml vendored
View File

@ -5,7 +5,7 @@
github:
- event: pull_request_review
action: submitted
state: approve
state: approved
start:
github: {}
success:

43
tests/fixtures/layouts/github-schema.yaml vendored Normal file
View File

@ -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: {}

4
tests/fixtures/layouts/pcre-deprecation-github.yaml vendored
View File

@ -3,9 +3,7 @@
manager: independent
trigger:
github:
- event: pull_request_review
action: submitted
state: approve
- event: push
ref: '(?!invalid)'
start:
github: {}

2
tests/fixtures/layouts/reviews-github.yaml vendored
View File

@ -5,7 +5,7 @@
github:
- event: pull_request_review
action: submitted
state: approve
state: approved
success:
github:
label:

2
tests/unit/test_github_crd.py
View File

@ -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)

39
tests/unit/test_github_driver.py
View File

@ -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))

204
zuul/driver/github/githubtrigger.py
View File

@ -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