Don't match branch protection rule patterns locally
Branch protection rules in github are fn patterns which are currently matched locally in zuul. This is error prone and can lead in edge cases to wrong matches resulting in wrong enqueue decisions into gate pipelines. When requesting branch protection rules in github we also can request the matching refs along with the rules. This is much safer since we can plain text match them against the change branch. Change-Id: Ic995d4b2e16a5d741f0209fa9236959d8f4d10b9
This commit is contained in:
parent
b2f6d48cc5
commit
b9c2d25dce
|
@ -32,7 +32,6 @@ paho-mqtt
|
||||||
cherrypy
|
cherrypy
|
||||||
ws4py
|
ws4py
|
||||||
routes
|
routes
|
||||||
pathspec
|
|
||||||
jsonpath-rw
|
jsonpath-rw
|
||||||
urllib3!=1.25.4,!=1.25.5 # https://github.com/urllib3/urllib3/pull/1684
|
urllib3!=1.25.4,!=1.25.5 # https://github.com/urllib3/urllib3/pull/1684
|
||||||
cheroot!=8.1.*,!=8.2.*,!=8.3.0 # https://github.com/cherrypy/cheroot/issues/263
|
cheroot!=8.1.*,!=8.2.*,!=8.3.0 # https://github.com/cherrypy/cheroot/issues/263
|
||||||
|
|
|
@ -26,11 +26,28 @@ class FakePageInfo(ObjectType):
|
||||||
return False
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
class FakeMatchingRef(ObjectType):
|
||||||
|
name = String()
|
||||||
|
|
||||||
|
def resolve_name(parent, info):
|
||||||
|
return parent
|
||||||
|
|
||||||
|
|
||||||
|
class FakeMatchingRefs(ObjectType):
|
||||||
|
nodes = List(FakeMatchingRef)
|
||||||
|
|
||||||
|
def resolve_nodes(parent, info):
|
||||||
|
# To simplify tests just return the pattern and a bogus ref that should
|
||||||
|
# not disturb zuul.
|
||||||
|
return [parent.pattern, 'bogus-ref']
|
||||||
|
|
||||||
|
|
||||||
class FakeBranchProtectionRule(ObjectType):
|
class FakeBranchProtectionRule(ObjectType):
|
||||||
pattern = String()
|
pattern = String()
|
||||||
requiredStatusCheckContexts = List(String)
|
requiredStatusCheckContexts = List(String)
|
||||||
requiresApprovingReviews = Boolean()
|
requiresApprovingReviews = Boolean()
|
||||||
requiresCodeOwnerReviews = Boolean()
|
requiresCodeOwnerReviews = Boolean()
|
||||||
|
matchingRefs = Field(FakeMatchingRefs, first=Int())
|
||||||
|
|
||||||
def resolve_pattern(parent, info):
|
def resolve_pattern(parent, info):
|
||||||
return parent.pattern
|
return parent.pattern
|
||||||
|
@ -44,6 +61,9 @@ class FakeBranchProtectionRule(ObjectType):
|
||||||
def resolve_requiresCodeOwnerReviews(parent, info):
|
def resolve_requiresCodeOwnerReviews(parent, info):
|
||||||
return parent.require_codeowners_review
|
return parent.require_codeowners_review
|
||||||
|
|
||||||
|
def resolve_matchingRefs(parent, info, first=None):
|
||||||
|
return parent
|
||||||
|
|
||||||
|
|
||||||
class FakeBranchProtectionRules(ObjectType):
|
class FakeBranchProtectionRules(ObjectType):
|
||||||
nodes = List(FakeBranchProtectionRule)
|
nodes = List(FakeBranchProtectionRule)
|
||||||
|
|
|
@ -1635,7 +1635,8 @@ class GithubConnection(BaseConnection):
|
||||||
|
|
||||||
# For performance reasons fetch all needed data for canMerge upfront
|
# For performance reasons fetch all needed data for canMerge upfront
|
||||||
# using a single graphql call.
|
# using a single graphql call.
|
||||||
canmerge_data = self.graphql_client.fetch_canmerge(github, change)
|
canmerge_data = self.graphql_client.fetch_canmerge(
|
||||||
|
github, change, zuul_event_id=event)
|
||||||
|
|
||||||
# If the PR is a draft it cannot be merged.
|
# If the PR is a draft it cannot be merged.
|
||||||
if canmerge_data.get('isDraft', False):
|
if canmerge_data.get('isDraft', False):
|
||||||
|
|
|
@ -13,9 +13,10 @@
|
||||||
# under the License.
|
# under the License.
|
||||||
import logging
|
import logging
|
||||||
|
|
||||||
import pathspec
|
|
||||||
from pkg_resources import resource_string
|
from pkg_resources import resource_string
|
||||||
|
|
||||||
|
from zuul.lib.logutil import get_annotated_logger
|
||||||
|
|
||||||
|
|
||||||
def nested_get(d, *keys, default=None):
|
def nested_get(d, *keys, default=None):
|
||||||
temp = d
|
temp = d
|
||||||
|
@ -70,7 +71,8 @@ class GraphQLClient:
|
||||||
response = github.session.post(self.url, json=query)
|
response = github.session.post(self.url, json=query)
|
||||||
return response.json()
|
return response.json()
|
||||||
|
|
||||||
def fetch_canmerge(self, github, change):
|
def fetch_canmerge(self, github, change, zuul_event_id=None):
|
||||||
|
log = get_annotated_logger(self.log, zuul_event_id)
|
||||||
owner, repo = change.project.name.split('/')
|
owner, repo = change.project.name.split('/')
|
||||||
|
|
||||||
data = self._fetch_canmerge(github, owner, repo, change.number,
|
data = self._fetch_canmerge(github, owner, repo, change.number,
|
||||||
|
@ -81,14 +83,21 @@ class GraphQLClient:
|
||||||
# Find corresponding rule to our branch
|
# Find corresponding rule to our branch
|
||||||
rules = nested_get(repository, 'branchProtectionRules', 'nodes',
|
rules = nested_get(repository, 'branchProtectionRules', 'nodes',
|
||||||
default=[])
|
default=[])
|
||||||
matching_rule = None
|
|
||||||
for rule in rules:
|
# Filter branch protection rules for the one matching the change.
|
||||||
pattern = pathspec.patterns.GitWildMatchPattern(
|
matching_rules = [
|
||||||
rule.get('pattern'))
|
rule for rule in rules
|
||||||
match = pathspec.match_files([pattern], [change.branch])
|
for ref in nested_get(rule, 'matchingRefs', 'nodes', default=[])
|
||||||
if match:
|
if ref.get('name') == change.branch
|
||||||
matching_rule = rule
|
]
|
||||||
break
|
if len(matching_rules) > 1:
|
||||||
|
log.warn('More than one branch protection rules match change %s',
|
||||||
|
change)
|
||||||
|
return result
|
||||||
|
elif len(matching_rules) == 1:
|
||||||
|
matching_rule = matching_rules[0]
|
||||||
|
else:
|
||||||
|
matching_rule = None
|
||||||
|
|
||||||
# If there is a matching rule, get required status checks
|
# If there is a matching rule, get required status checks
|
||||||
if matching_rule:
|
if matching_rule:
|
||||||
|
|
|
@ -11,6 +11,11 @@ query canMergeData(
|
||||||
requiredStatusCheckContexts
|
requiredStatusCheckContexts
|
||||||
requiresApprovingReviews
|
requiresApprovingReviews
|
||||||
requiresCodeOwnerReviews
|
requiresCodeOwnerReviews
|
||||||
|
matchingRefs(first: 100) {
|
||||||
|
nodes {
|
||||||
|
name
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
pullRequest(number: $pull) {
|
pullRequest(number: $pull) {
|
||||||
|
|
|
@ -11,6 +11,11 @@ query canMergeData(
|
||||||
requiredStatusCheckContexts
|
requiredStatusCheckContexts
|
||||||
requiresApprovingReviews
|
requiresApprovingReviews
|
||||||
requiresCodeOwnerReviews
|
requiresCodeOwnerReviews
|
||||||
|
matchingRefs(first: 100) {
|
||||||
|
nodes {
|
||||||
|
name
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
pullRequest(number: $pull) {
|
pullRequest(number: $pull) {
|
||||||
|
|
Loading…
Reference in New Issue