Hi,
I have written a few patches to improve excuses in the following areas:
* HTML excuses will include a line explicitly defining whether people
need to act on the excuse or not. Intended for any contributor with
less than 20 hours of experience with Britney. See [1] for the
concrete messages
* The YAML excuses will have a similar information in a single enum
value.
* The YAML excuses will also have a per-policy verdict. This means
that consumers have a consistent way of determine the result of a
given policy.
Assuming no objections, I will merge this into master early next week.
~Niels
[1]
> +VERDICT2DESC = {
> + PolicyVerdict.PASS:
> + 'OK: Will attempt migration (Any information below is purely informational)',
> + PolicyVerdict.PASS_HINTED:
> + 'OK: Will attempt migration due to a hint (Any information below is purely informational)',
> + PolicyVerdict.REJECTED_TEMPORARILY:
> + 'WAITING: Waiting for test results, another package or too young (no action required now - check later)',
> + PolicyVerdict.REJECTED_WAITING_FOR_ANOTHER_ITEM:
> + 'WAITING: Waiting for another item to be ready to migrate (no action required now - check later)',
> + PolicyVerdict.REJECTED_BLOCKED_BY_ANOTHER_ITEM:
> + 'BLOCKED: Cannot due to another item, which is blocked (please check which dependencies are stuck)',
> + PolicyVerdict.REJECTED_NEEDS_APPROVAL:
> + 'BLOCKED: Needs an approval (either due to a freeze or due to the source suite)',
> + PolicyVerdict.REJECTED_CANNOT_DETERMINE_IF_PERMANENT:
> + 'BLOCKED: Maybe temporary, maybe blocked but Britney is missing information (check below or the buildds)',
> + PolicyVerdict.REJECTED_PERMANENTLY:
> + 'BLOCKED: Rejected/introduces a regression (please see below)'
> +}
From 79f16c45239fb3188ae231ccc86fabdecb151e4a Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Mon, 19 Dec 2016 17:59:54 +0000
Subject: [PATCH 1/8] Remove redundant if-else - if branch is always true
Signed-off-by: Niels Thykier <niels@thykier.net>
---
britney2/policies/policy.py | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/britney2/policies/policy.py b/britney2/policies/policy.py
index 7b100ad..5bbbf39 100644
--- a/britney2/policies/policy.py
+++ b/britney2/policies/policy.py
@@ -97,10 +97,8 @@ class BasePolicy(object):
pass
def apply_policy(self, general_policy_info, suite, source_name, source_data_tdist, source_data_srcdist, excuse):
- if self.policy_id not in general_policy_info:
- general_policy_info[self.policy_id] = pinfo = {}
- else:
- pinfo = general_policy_info[self.policy_id]
+ pinfo = {}
+ general_policy_info[self.policy_id] = pinfo
return self.apply_policy_impl(pinfo, suite, source_name, source_data_tdist, source_data_srcdist, excuse)
@abstractmethod
--
2.11.0
From 040c71a31c09d2d3215b60c7b4fcbf4a69f38025 Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Mon, 19 Dec 2016 18:09:45 +0000
Subject: [PATCH 2/8] tests/test_policy.py: refactor test cases
Signed-off-by: Niels Thykier <niels@thykier.net>
---
tests/test_policy.py | 135 ++++++++++++++++++++++-----------------------------
1 file changed, 58 insertions(+), 77 deletions(-)
diff --git a/tests/test_policy.py b/tests/test_policy.py
index a51b557..dd468d4 100644
--- a/tests/test_policy.py
+++ b/tests/test_policy.py
@@ -51,83 +51,76 @@ def create_policy_objects(source_name, target_version, source_version):
)
+def apply_policy(policy, expected_verdict, src_name, *, suite='unstable', source_version='1.0', target_version='2.0'):
+ src_t, src_u, excuse, policy_info = create_policy_objects(src_name, source_version, target_version)
+ verdict = policy.apply_policy(policy_info, suite, src_name, src_t, src_u, excuse)
+ assert verdict == expected_verdict
+ return policy_info[policy.policy_id]
+
+
class TestRCBugsPolicy(unittest.TestCase):
def test_no_bugs(self):
src_name = 'has-no-bugs'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('rc-bugs/basic', RCBugPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.PASS
- assert set(policy_info['rc-bugs']['unique-source-bugs']) == set()
- assert set(policy_info['rc-bugs']['unique-target-bugs']) == set()
- assert set(policy_info['rc-bugs']['shared-bugs']) == set()
+ bug_policy_info = apply_policy(policy, PolicyVerdict.PASS, src_name)
+ assert set(bug_policy_info['unique-source-bugs']) == set()
+ assert set(bug_policy_info['unique-target-bugs']) == set()
+ assert set(bug_policy_info['shared-bugs']) == set()
def test_regression(self):
src_name = 'regression'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('rc-bugs/basic', RCBugPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.REJECTED_PERMANENTLY
- assert set(policy_info['rc-bugs']['unique-source-bugs']) == {'123458'}
- assert set(policy_info['rc-bugs']['unique-target-bugs']) == set()
- assert set(policy_info['rc-bugs']['shared-bugs']) == set()
+ bug_policy_info = apply_policy(policy, PolicyVerdict.REJECTED_PERMANENTLY, src_name)
+ assert set(bug_policy_info['unique-source-bugs']) == {'123458'}
+ assert set(bug_policy_info['unique-target-bugs']) == set()
+ assert set(bug_policy_info['shared-bugs']) == set()
def test_regression_but_fixes_more_bugs(self):
src_name = 'regression-but-fixes-more-bugs'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('rc-bugs/basic', RCBugPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.REJECTED_PERMANENTLY
- assert set(policy_info['rc-bugs']['unique-source-bugs']) == {'100003'}
- assert set(policy_info['rc-bugs']['unique-target-bugs']) == {'100001', '100002'}
- assert set(policy_info['rc-bugs']['shared-bugs']) == {'100000'}
+ bug_policy_info = apply_policy(policy, PolicyVerdict.REJECTED_PERMANENTLY, src_name)
+ assert set(bug_policy_info['unique-source-bugs']) == {'100003'}
+ assert set(bug_policy_info['unique-target-bugs']) == {'100001', '100002'}
+ assert set(bug_policy_info['shared-bugs']) == {'100000'}
def test_not_a_regression(self):
src_name = 'not-a-regression'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('rc-bugs/basic', RCBugPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.PASS
- assert set(policy_info['rc-bugs']['unique-source-bugs']) == set()
- assert set(policy_info['rc-bugs']['unique-target-bugs']) == set()
- assert set(policy_info['rc-bugs']['shared-bugs']) == {'123457'}
+ bug_policy_info = apply_policy(policy, PolicyVerdict.PASS, src_name)
+ assert set(bug_policy_info['unique-source-bugs']) == set()
+ assert set(bug_policy_info['unique-target-bugs']) == set()
+ assert set(bug_policy_info['shared-bugs']) == {'123457'}
def test_improvement(self):
src_name = 'fixes-bug'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('rc-bugs/basic', RCBugPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.PASS
- assert set(policy_info['rc-bugs']['unique-source-bugs']) == set()
- assert set(policy_info['rc-bugs']['unique-target-bugs']) == {'123456'}
- assert set(policy_info['rc-bugs']['shared-bugs']) == set()
+ bug_policy_info = apply_policy(policy, PolicyVerdict.PASS, src_name)
+ assert set(bug_policy_info['unique-source-bugs']) == set()
+ assert set(bug_policy_info['unique-target-bugs']) == {'123456'}
+ assert set(bug_policy_info['shared-bugs']) == set()
def test_regression_with_hint(self):
src_name = 'regression'
hints = ['ignore-rc-bugs 123458 regression/2.0']
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('rc-bugs/basic', RCBugPolicy, hints=hints)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.PASS_HINTED
- assert set(policy_info['rc-bugs']['ignored-bugs']['bugs']) == {'123458'}
- assert policy_info['rc-bugs']['ignored-bugs']['issued-by'] == TEST_HINTER
- assert set(policy_info['rc-bugs']['unique-source-bugs']) == set()
- assert set(policy_info['rc-bugs']['unique-target-bugs']) == set()
- assert set(policy_info['rc-bugs']['shared-bugs']) == set()
+ bug_policy_info = apply_policy(policy, PolicyVerdict.PASS_HINTED, src_name)
+ assert set(bug_policy_info['ignored-bugs']['bugs']) == {'123458'}
+ assert bug_policy_info['ignored-bugs']['issued-by'] == TEST_HINTER
+ assert set(bug_policy_info['unique-source-bugs']) == set()
+ assert set(bug_policy_info['unique-target-bugs']) == set()
+ assert set(bug_policy_info['shared-bugs']) == set()
def test_regression_but_fixes_more_bugs_bad_hint(self):
src_name = 'regression-but-fixes-more-bugs'
hints = ['ignore-rc-bugs 100000 regression-but-fixes-more-bugs/2.0']
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('rc-bugs/basic', RCBugPolicy, hints=hints)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.REJECTED_PERMANENTLY
- assert set(policy_info['rc-bugs']['unique-source-bugs']) == {'100003'}
- assert set(policy_info['rc-bugs']['unique-target-bugs']) == {'100001', '100002'}
- assert set(policy_info['rc-bugs']['ignored-bugs']['bugs']) == {'100000'}
- assert policy_info['rc-bugs']['ignored-bugs']['issued-by'] == TEST_HINTER
- assert set(policy_info['rc-bugs']['shared-bugs']) == set()
+ bug_policy_info = apply_policy(policy, PolicyVerdict.REJECTED_PERMANENTLY, src_name)
+ assert set(bug_policy_info['unique-source-bugs']) == {'100003'}
+ assert set(bug_policy_info['unique-target-bugs']) == {'100001', '100002'}
+ assert set(bug_policy_info['ignored-bugs']['bugs']) == {'100000'}
+ assert bug_policy_info['ignored-bugs']['issued-by'] == TEST_HINTER
+ assert set(bug_policy_info['shared-bugs']) == set()
class TestAgePolicy(unittest.TestCase):
@@ -146,13 +139,11 @@ class TestAgePolicy(unittest.TestCase):
try:
src_name = 'unlisted-source-package'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('age/missing-age-file', AgePolicy, TestAgePolicy.DEFAULT_MIN_DAYS)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
+ age_policy_info = apply_policy(policy, PolicyVerdict.REJECTED_TEMPORARILY, src_name)
assert os.path.exists(age_file)
- assert verdict == PolicyVerdict.REJECTED_TEMPORARILY
- assert policy_info['age']['age-requirement'] == TestAgePolicy.DEFAULT_MIN_DAYS[DEFAULT_URGENCY]
- assert policy_info['age']['current-age'] == 0
+ assert age_policy_info['age-requirement'] == TestAgePolicy.DEFAULT_MIN_DAYS[DEFAULT_URGENCY]
+ assert age_policy_info['current-age'] == 0
finally:
if os.path.exists(age_file):
os.unlink(age_file)
@@ -162,50 +153,40 @@ class TestPiupartsPolicy(unittest.TestCase):
def test_passes(self):
src_name = 'pass'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('piuparts/basic', PiupartsPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.PASS
- assert policy_info['piuparts']['test-results'] == 'pass'
- assert policy_info['piuparts']['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/p/pass.html'
+ piu_policy_info = apply_policy(policy, PolicyVerdict.PASS, src_name)
+ assert piu_policy_info['test-results'] == 'pass'
+ assert piu_policy_info['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/p/pass.html'
def test_regression(self):
src_name = 'regression'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('piuparts/basic', PiupartsPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.REJECTED_PERMANENTLY
- assert policy_info['piuparts']['test-results'] == 'regression'
- assert policy_info['piuparts']['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/r/regression.html'
+ piu_policy_info = apply_policy(policy, PolicyVerdict.REJECTED_PERMANENTLY, src_name)
+ assert piu_policy_info['test-results'] == 'regression'
+ assert piu_policy_info['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/r/regression.html'
def test_regression_hinted(self):
src_name = 'regression'
hints = ['ignore-piuparts regression/2.0']
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('piuparts/basic', PiupartsPolicy, hints=hints)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.PASS_HINTED
- assert policy_info['piuparts']['test-results'] == 'regression'
- assert policy_info['piuparts']['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/r/regression.html'
- assert policy_info['piuparts']['ignored-piuparts']['issued-by'] == TEST_HINTER
+ piu_policy_info = apply_policy(policy, PolicyVerdict.PASS_HINTED, src_name)
+ assert piu_policy_info['test-results'] == 'regression'
+ assert piu_policy_info['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/r/regression.html'
+ assert piu_policy_info['ignored-piuparts']['issued-by'] == TEST_HINTER
def test_not_tested_yet(self):
src_name = 'not-tested-yet'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('piuparts/basic', PiupartsPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.REJECTED_TEMPORARILY
- assert policy_info['piuparts']['test-results'] == 'waiting-for-test-results'
- assert policy_info['piuparts']['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/n/not-tested-yet.html'
+ piu_policy_info = apply_policy(policy, PolicyVerdict.REJECTED_TEMPORARILY, src_name)
+ assert piu_policy_info['test-results'] == 'waiting-for-test-results'
+ assert piu_policy_info['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/n/not-tested-yet.html'
def test_failed_not_regression(self):
src_name = 'failed-not-regression'
- src_t, src_u, excuse, policy_info = create_policy_objects(src_name, '1.0', '2.0')
policy = initialize_policy('piuparts/basic', PiupartsPolicy)
- verdict = policy.apply_policy(policy_info, 'unstable', src_name, src_t, src_u, excuse)
- assert verdict == PolicyVerdict.PASS
- assert policy_info['piuparts']['test-results'] == 'failed'
- assert policy_info['piuparts']['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/f/failed-not-regression.html'
+ piu_policy_info = apply_policy(policy, PolicyVerdict.PASS, src_name)
+ assert piu_policy_info['test-results'] == 'failed'
+ assert piu_policy_info['piuparts-test-url'] == 'https://piuparts.debian.org/sid/source/f/failed-not-regression.html'
if __name__ == '__main__':
unittest.main()
--
2.11.0
From 289fbcdbe445e5c39d6262d3d1f8c4fc9e49ab5f Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Mon, 19 Dec 2016 18:27:38 +0000
Subject: [PATCH 3/8] Add a verdict field to all policies with the actual
verdict
Signed-off-by: Niels Thykier <niels@thykier.net>
---
britney2/policies/policy.py | 6 +++++-
tests/test_policy.py | 4 +++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/britney2/policies/policy.py b/britney2/policies/policy.py
index 5bbbf39..d5ad271 100644
--- a/britney2/policies/policy.py
+++ b/britney2/policies/policy.py
@@ -99,7 +99,11 @@ class BasePolicy(object):
def apply_policy(self, general_policy_info, suite, source_name, source_data_tdist, source_data_srcdist, excuse):
pinfo = {}
general_policy_info[self.policy_id] = pinfo
- return self.apply_policy_impl(pinfo, suite, source_name, source_data_tdist, source_data_srcdist, excuse)
+ verdict = self.apply_policy_impl(pinfo, suite, source_name, source_data_tdist, source_data_srcdist, excuse)
+ # The base policy provides this field, so the subclass should leave it blank
+ assert 'verdict' not in pinfo
+ pinfo['verdict'] = verdict.name
+ return verdict
@abstractmethod
def apply_policy_impl(self, policy_info, suite, source_name, source_data_tdist, source_data_srcdist, excuse): # pragma: no cover
diff --git a/tests/test_policy.py b/tests/test_policy.py
index dd468d4..1e6bfa4 100644
--- a/tests/test_policy.py
+++ b/tests/test_policy.py
@@ -54,8 +54,10 @@ def create_policy_objects(source_name, target_version, source_version):
def apply_policy(policy, expected_verdict, src_name, *, suite='unstable', source_version='1.0', target_version='2.0'):
src_t, src_u, excuse, policy_info = create_policy_objects(src_name, source_version, target_version)
verdict = policy.apply_policy(policy_info, suite, src_name, src_t, src_u, excuse)
+ pinfo = policy_info[policy.policy_id]
assert verdict == expected_verdict
- return policy_info[policy.policy_id]
+ assert pinfo['verdict'] == expected_verdict.name
+ return pinfo
class TestRCBugsPolicy(unittest.TestCase):
--
2.11.0
From 8d8f1053b938482c9fe474f27a0f14be21147f49 Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Wed, 8 Feb 2017 17:38:49 +0000
Subject: [PATCH 4/8] britney: Don't show "fake" packages in excuses
They are implementation details and should not appear in the excuses.
Signed-off-by: Niels Thykier <niels@thykier.net>
---
britney.py | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/britney.py b/britney.py
index fa1740e..ceab846 100755
--- a/britney.py
+++ b/britney.py
@@ -1259,8 +1259,12 @@ class Britney(object):
the object attribute excuses.
"""
- # retrieve the source packages for testing (if available) and suite
source_u = self.sources[suite][src]
+ if source_u.is_fakesrc:
+ # it is a fake package created to satisfy Britney implementation details; silently ignore it
+ return False
+
+ # retrieve the source packages for testing (if available) and suite
if src in self.sources['testing']:
source_t = self.sources['testing'][src]
# if testing and unstable have the same version, then this is a candidate for binary-NMUs only
@@ -1291,11 +1295,6 @@ class Britney(object):
# the starting point is that we will update the candidate
excuse.is_valid = True
- # check if the source package really exists or if it is a fake one
- if source_u.is_fakesrc:
- excuse.addhtml("%s source package doesn't exist" % (src))
- excuse.is_valid = False
-
# if there is a `remove' hint and the requested version is the same as the
# version in testing, then stop here and return False
for hint in self.hints.search('remove', package=src):
--
2.11.0
From fd80bdc571517693f69078ce6c9f9b4fa9543b1e Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Wed, 8 Feb 2017 17:49:30 +0000
Subject: [PATCH 5/8] Only show the first (relevant) remove hint in excuses
In the unlike case that there are multiple removal hints, showing
the first valid hint should be sufficient.
Signed-off-by: Niels Thykier <niels@thykier.net>
---
britney.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/britney.py b/britney.py
index ceab846..6a7262f 100755
--- a/britney.py
+++ b/britney.py
@@ -1304,6 +1304,7 @@ class Britney(object):
excuse.addhtml("Removal request by %s" % (hint.user))
excuse.addhtml("Trying to remove package, not update it")
excuse.is_valid = False
+ break
# check if there is a `block' or `block-udeb' hint for this package, or a `block-all source' hint
blocked = {}
--
2.11.0
From 03c78d8e085cade9ac7a9a35d839d4e5e3e3e34d Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Wed, 8 Feb 2017 18:14:29 +0000
Subject: [PATCH 6/8] Replace dontinvalidate with forced
Signed-off-by: Niels Thykier <niels@thykier.net>
---
britney.py | 7 +++----
britney2/excuse.py | 11 ++++-------
britney2/utils.py | 14 +++++++-------
3 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/britney.py b/britney.py
index 6a7262f..7d754b1 100755
--- a/britney.py
+++ b/britney.py
@@ -1498,11 +1498,10 @@ class Britney(object):
# check if there is a `force' hint for this package, which allows it to go in even if it is not updateable
forces = self.hints.search('force', package=src, version=source_u.version)
if forces:
- excuse.dontinvalidate = True
- if not excuse.is_valid:
+ # force() updates the final verdict for us
+ changed_state = excuse.force()
+ if changed_state:
excuse.addhtml("Should ignore, but forced by %s" % (forces[0].user))
- excuse.force()
- excuse.is_valid = True
self.excuses[excuse.name] = excuse
return excuse.is_valid
diff --git a/britney2/excuse.py b/britney2/excuse.py
index d7afd18..706e95a 100644
--- a/britney2/excuse.py
+++ b/britney2/excuse.py
@@ -48,7 +48,6 @@ class Excuse(object):
self.mindays = None
self.section = None
self._is_valid = False
- self._dontinvalidate = False
self.needs_approval = False
self.hints = []
self.forced = False
@@ -80,13 +79,7 @@ class Excuse(object):
def is_valid(self, value):
self._is_valid = value
- @property
- def dontinvalidate(self):
- return self._dontinvalidate
- @dontinvalidate.setter
- def dontinvalidate(self, value):
- self._dontinvalidate = value
def set_vers(self, tver, uver):
"""Set the testing and unstable versions"""
@@ -127,6 +120,10 @@ class Excuse(object):
def force(self):
"""Add force hint"""
self.forced = True
+ if not self._is_valid:
+ self._is_valid = True
+ return True
+ return False
def addhtml(self, note):
"""Add a note in HTML"""
diff --git a/britney2/utils.py b/britney2/utils.py
index c819362..3827aa0 100644
--- a/britney2/utils.py
+++ b/britney2/utils.py
@@ -802,14 +802,14 @@ def invalidate_excuses(excuses, valid, invalid):
continue
# loop on the reverse dependencies
for x in revdeps[ename]:
- # if the item is valid and it is marked as `dontinvalidate', skip the item
- if x in valid and excuses[x].dontinvalidate:
- continue
-
- # otherwise, invalidate the dependency and mark as invalidated and
- # remove the depending excuses
- excuses[x].invalidate_dep(ename)
if x in valid:
+ # if the item is valid and it is marked as `forced', skip the item
+ if excuses[x].forced:
+ continue
+
+ # otherwise, invalidate the dependency and mark as invalidated and
+ # remove the depending excuses
+ excuses[x].invalidate_dep(ename)
p = valid.index(x)
invalid.append(valid.pop(p))
excuses[x].addhtml("Invalidated by dependency")
--
2.11.0
From a80d1c2d83b7cc41b5ad21f5f702ec0f6fe5fe44 Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Wed, 8 Feb 2017 19:01:41 +0000
Subject: [PATCH 7/8] Aggregate all migration decisions and present it in
excuses
With this change, Britney can now provide a very brief summary of the
migration via one single value (YAML) or line (HTML). This solves two
issues:
* It provides an aggregated version of the policy decision without
having to loop over all policies (and even those would not give
a full verdict on their own as not all rejections come from
policies)
* It enables a simple way to inform readers of the HTML excuses of
whether a rejection is permanent or not. This should hopefully
make it easier for contributors to understand Britney and react
more pro-actively.
Signed-off-by: Niels Thykier <niels@thykier.net>
---
britney.py | 30 ++++++++++++++++--------------
britney2/excuse.py | 37 ++++++++++++++++++++++++++++++-------
britney2/utils.py | 4 +++-
3 files changed, 49 insertions(+), 22 deletions(-)
diff --git a/britney.py b/britney.py
index 7d754b1..a8b4e31 100755
--- a/britney.py
+++ b/britney.py
@@ -1085,7 +1085,7 @@ class Britney(object):
self.excuses[excuse.name] = excuse
return False
- excuse.is_valid = True
+ excuse.policy_verdict = PolicyVerdict.PASS
self.excuses[excuse.name] = excuse
return True
@@ -1237,7 +1237,7 @@ class Britney(object):
# if there is nothing wrong and there is something worth doing, this is a valid candidate
if not anywrongver and anyworthdoing:
- excuse.is_valid = True
+ excuse.policy_verdict = PolicyVerdict.PASS
self.excuses[excuse.name] = excuse
return True
# else if there is something worth doing (but something wrong, too) this package won't be considered
@@ -1293,7 +1293,7 @@ class Britney(object):
return False
# the starting point is that we will update the candidate
- excuse.is_valid = True
+ excuse.policy_verdict = PolicyVerdict.PASS
# if there is a `remove' hint and the requested version is the same as the
# version in testing, then stop here and return False
@@ -1303,7 +1303,7 @@ class Britney(object):
excuse.add_hint(hint)
excuse.addhtml("Removal request by %s" % (hint.user))
excuse.addhtml("Trying to remove package, not update it")
- excuse.is_valid = False
+ excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
break
# check if there is a `block' or `block-udeb' hint for this package, or a `block-all source' hint
@@ -1358,7 +1358,7 @@ class Britney(object):
else:
excuse.addhtml("NEEDS APPROVAL BY RM")
excuse.addreason("block")
- excuse.is_valid = False
+ excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
# at this point, we check the status of the builds on all the supported architectures
# to catch the out-of-date ones
@@ -1404,7 +1404,7 @@ class Britney(object):
# we would not need to be forgiving at
# all. However, due to how arch:all packages
# are handled, we do run into occasionally.
- excuse.is_valid = False
+ excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
# if there are out-of-date packages, warn about them in the excuse and set excuse.is_valid
# to False to block the update; if the architecture where the package is out-of-date is
@@ -1434,9 +1434,9 @@ class Britney(object):
if self.options.ignore_cruft:
text = text + " (but ignoring cruft, so nevermind)"
else:
- excuse.is_valid = False
+ excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
else:
- excuse.is_valid = False
+ excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
excuse.missing_build_on_arch(arch)
excuse.addhtml(text)
@@ -1445,21 +1445,20 @@ class Britney(object):
if not source_u.binaries:
excuse.addhtml("%s has no binaries on any arch" % src)
excuse.addreason("no-binaries")
- excuse.is_valid = False
+ excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
# if the suite is unstable, then we have to check the urgency and the minimum days of
# permanence in unstable before updating testing; if the source package is too young,
# the check fails and we set is_valid to False to block the update; consider
# the age-days hint, if specified for the package
+ policy_verdict = excuse.policy_verdict
policy_info = excuse.policy_info
- policy_verdict = PolicyVerdict.PASS
for policy in self.policies:
if suite in policy.applicable_suites:
v = policy.apply_policy(policy_info, suite, src, source_t, source_u, excuse)
if v.value > policy_verdict.value:
policy_verdict = v
- if policy_verdict.is_rejected:
- excuse.is_valid = False
+ excuse.policy_verdict = policy_verdict
if suite in ('pu', 'tpu') and source_t:
# o-o-d(ish) checks for (t-)p-u
@@ -1490,7 +1489,7 @@ class Britney(object):
text = text + " (but %s isn't keeping up, so never mind)" % (arch)
excuse.missing_build_on_ood_arch(arch)
else:
- excuse.is_valid = False
+ excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
excuse.missing_build_on_arch(arch)
excuse.addhtml(text)
@@ -1585,7 +1584,10 @@ class Britney(object):
excuse.addhtml("Removal request by %s" % (hint.user))
excuse.addhtml("Package is broken, will try to remove")
excuse.add_hint(hint)
- excuse.is_valid = True
+ # Using "PASS" here as "Created by a hint" != "accepted due to hint". In a future
+ # where there might be policy checks on removals, it would make sense to distinguish
+ # those two states. Not sure that future will ever be.
+ excuse.policy_verdict = PolicyVerdict.PASS
excuses[excuse.name] = excuse
# extract the not considered packages, which are in the excuses but not in upgrade_me
diff --git a/britney2/excuse.py b/britney2/excuse.py
index 706e95a..e854277 100644
--- a/britney2/excuse.py
+++ b/britney2/excuse.py
@@ -17,6 +17,7 @@
from collections import defaultdict
import re
+from britney2.policies.policy import PolicyVerdict
class Excuse(object):
"""Excuse class
@@ -51,6 +52,7 @@ class Excuse(object):
self.needs_approval = False
self.hints = []
self.forced = False
+ self._policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
self.invalid_deps = []
self.deps = {}
@@ -73,13 +75,19 @@ class Excuse(object):
@property
def is_valid(self):
- return self._is_valid
-
- @is_valid.setter
- def is_valid(self, value):
- self._is_valid = value
+ return False if self._policy_verdict.is_rejected else True
+ @property
+ def policy_verdict(self):
+ return self._policy_verdict
+ @policy_verdict.setter
+ def policy_verdict(self, value):
+ if value.is_rejected and self.forced:
+ # By virtue of being forced, the item was hinted to
+ # undo the rejection
+ value = PolicyVerdict.PASS_HINTED
+ self._policy_verdict = value
def set_vers(self, tver, uver):
"""Set the testing and unstable versions"""
@@ -120,8 +128,8 @@ class Excuse(object):
def force(self):
"""Add force hint"""
self.forced = True
- if not self._is_valid:
- self._is_valid = True
+ if self._policy_verdict.is_rejected:
+ self._policy_verdict = PolicyVerdict.PASS_HINTED
return True
return False
@@ -144,10 +152,24 @@ class Excuse(object):
def add_hint(self, hint):
self.hints.append(hint)
+ def _format_verdict_summary(self):
+ verdict = self._policy_verdict
+ if not verdict.is_rejected:
+ msg = 'OK: Will attempt migration'
+ if verdict == PolicyVerdict.PASS_HINTED:
+ msg = 'OK: Will attempt migration due to a hint'
+ msg += " (Any information below is purely informational)"
+ return msg
+ if verdict == PolicyVerdict.REJECTED_PERMANENTLY:
+ msg = "BLOCKED: Will not migrate (Please review if it introduces a regression or needs approval/unblock)"
+ return msg
+ return "TEMP-BLOCKED: Waiting for test results, another package or too young (no action required at this time)"
+
def html(self):
"""Render the excuse in HTML"""
res = "<a id=\"%s\" name=\"%s\">%s</a> (%s to %s)\n<ul>\n" % \
(self.name, self.name, self.name, self.ver[0], self.ver[1])
+ res += "<li>Migration status: %s" % self._format_verdict_summary()
if self.maint:
res = res + "<li>Maintainer: %s\n" % (self.maint)
if self.section and self.section.find("/") > -1:
@@ -210,6 +232,7 @@ class Excuse(object):
excusedata["excuses"] = self._text()
excusedata["item-name"] = self.name
excusedata["source"] = source
+ excusedata["migration-policy-verdict"] = self._policy_verdict
excusedata["old-version"] = self.ver[0]
excusedata["new-version"] = self.ver[1]
if self.maint:
diff --git a/britney2/utils.py b/britney2/utils.py
index 3827aa0..b518dd9 100644
--- a/britney2/utils.py
+++ b/britney2/utils.py
@@ -39,6 +39,7 @@ from britney2.consts import (VERSION, PROVIDES, DEPENDS, CONFLICTS,
SOURCE, MAINTAINER, MULTIARCH,
ESSENTIAL)
from britney2.migrationitem import MigrationItem, UnversionnedMigrationItem
+from britney2.policies.policy import PolicyVerdict
def ifilter_except(container, iterable=None):
@@ -814,7 +815,8 @@ def invalidate_excuses(excuses, valid, invalid):
invalid.append(valid.pop(p))
excuses[x].addhtml("Invalidated by dependency")
excuses[x].addreason("depends")
- excuses[x].is_valid = False
+ if excuses[x].policy_verdict.value < PolicyVerdict.REJECTED_TEMPORARILY.value:
+ excuses[x].policy_verdict = PolicyVerdict.REJECTED_TEMPORARILY
def compile_nuninst(binaries_t, inst_tester, architectures, nobreakall_arches):
--
2.11.0
From 10333ee45da6664ace7c6a5a9a5d68024d1ec07f Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Wed, 8 Feb 2017 20:02:16 +0000
Subject: [PATCH 8/8] Add more verdicts to provide more detailed excuses
Signed-off-by: Niels Thykier <niels@thykier.net>
---
britney.py | 6 +++---
britney2/excuse.py | 33 +++++++++++++++++++++++----------
britney2/policies/policy.py | 28 +++++++++++++++++++++++++++-
britney2/utils.py | 9 +++++++--
4 files changed, 60 insertions(+), 16 deletions(-)
diff --git a/britney.py b/britney.py
index a8b4e31..bc0120f 100755
--- a/britney.py
+++ b/britney.py
@@ -1358,7 +1358,7 @@ class Britney(object):
else:
excuse.addhtml("NEEDS APPROVAL BY RM")
excuse.addreason("block")
- excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
+ excuse.policy_verdict = PolicyVerdict.REJECTED_NEEDS_APPROVAL
# at this point, we check the status of the builds on all the supported architectures
# to catch the out-of-date ones
@@ -1436,7 +1436,7 @@ class Britney(object):
else:
excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
else:
- excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
+ excuse.policy_verdict = PolicyVerdict.REJECTED_CANNOT_DETERMINE_IF_PERMANENT
excuse.missing_build_on_arch(arch)
excuse.addhtml(text)
@@ -1489,7 +1489,7 @@ class Britney(object):
text = text + " (but %s isn't keeping up, so never mind)" % (arch)
excuse.missing_build_on_ood_arch(arch)
else:
- excuse.policy_verdict = PolicyVerdict.REJECTED_PERMANENTLY
+ excuse.policy_verdict = PolicyVerdict.REJECTED_CANNOT_DETERMINE_IF_PERMANENT
excuse.missing_build_on_arch(arch)
excuse.addhtml(text)
diff --git a/britney2/excuse.py b/britney2/excuse.py
index e854277..a1ce27a 100644
--- a/britney2/excuse.py
+++ b/britney2/excuse.py
@@ -19,6 +19,26 @@ import re
from britney2.policies.policy import PolicyVerdict
+VERDICT2DESC = {
+ PolicyVerdict.PASS:
+ 'OK: Will attempt migration (Any information below is purely informational)',
+ PolicyVerdict.PASS_HINTED:
+ 'OK: Will attempt migration due to a hint (Any information below is purely informational)',
+ PolicyVerdict.REJECTED_TEMPORARILY:
+ 'WAITING: Waiting for test results, another package or too young (no action required now - check later)',
+ PolicyVerdict.REJECTED_WAITING_FOR_ANOTHER_ITEM:
+ 'WAITING: Waiting for another item to be ready to migrate (no action required now - check later)',
+ PolicyVerdict.REJECTED_BLOCKED_BY_ANOTHER_ITEM:
+ 'BLOCKED: Cannot due to another item, which is blocked (please check which dependencies are stuck)',
+ PolicyVerdict.REJECTED_NEEDS_APPROVAL:
+ 'BLOCKED: Needs an approval (either due to a freeze or due to the source suite)',
+ PolicyVerdict.REJECTED_CANNOT_DETERMINE_IF_PERMANENT:
+ 'BLOCKED: Maybe temporary, maybe blocked but Britney is missing information (check below or the buildds)',
+ PolicyVerdict.REJECTED_PERMANENTLY:
+ 'BLOCKED: Rejected/introduces a regression (please see below)'
+}
+
+
class Excuse(object):
"""Excuse class
@@ -154,16 +174,9 @@ class Excuse(object):
def _format_verdict_summary(self):
verdict = self._policy_verdict
- if not verdict.is_rejected:
- msg = 'OK: Will attempt migration'
- if verdict == PolicyVerdict.PASS_HINTED:
- msg = 'OK: Will attempt migration due to a hint'
- msg += " (Any information below is purely informational)"
- return msg
- if verdict == PolicyVerdict.REJECTED_PERMANENTLY:
- msg = "BLOCKED: Will not migrate (Please review if it introduces a regression or needs approval/unblock)"
- return msg
- return "TEMP-BLOCKED: Waiting for test results, another package or too young (no action required at this time)"
+ if verdict in VERDICT2DESC:
+ return VERDICT2DESC[verdict]
+ return "UNKNOWN: Missing description for {0} - Please file a bug against Britney".format(verdict.name)
def html(self):
"""Render the excuse in HTML"""
diff --git a/britney2/policies/policy.py b/britney2/policies/policy.py
index d5ad271..6e2c6e1 100644
--- a/britney2/policies/policy.py
+++ b/britney2/policies/policy.py
@@ -27,15 +27,41 @@ class PolicyVerdict(Enum):
"""
REJECTED_TEMPORARILY = 3
"""
+ The migration item is temporarily unable to migrate due to another item. The other item is temporarily blocked.
+ """
+ REJECTED_WAITING_FOR_ANOTHER_ITEM = 4
+ """
+ The migration item is permanently unable to migrate due to another item. The other item is permanently blocked.
+ """
+ REJECTED_BLOCKED_BY_ANOTHER_ITEM = 5
+ """
+ The migration item needs approval to migrate
+ """
+ REJECTED_NEEDS_APPROVAL = 6
+ """
+ The migration item is blocked, but there is not enough information to determine
+ if this issue is permanent or temporary
+ """
+ REJECTED_CANNOT_DETERMINE_IF_PERMANENT = 7
+ """
The migration item did not pass the policy and the failure is believed
to be uncorrectable (i.e. a hint or a new version is needed)
"""
- REJECTED_PERMANENTLY = 4
+ REJECTED_PERMANENTLY = 8
@property
def is_rejected(self):
return True if self.name.startswith('REJECTED') else False
+ def is_blocked(self):
+ """Whether the item (probably) needs a fix or manual assistance to migrate"""
+ return self in {
+ PolicyVerdict.REJECTED_BLOCKED_BY_ANOTHER_ITEM,
+ PolicyVerdict.REJECTED_NEEDS_APPROVAL,
+ PolicyVerdict.REJECTED_CANNOT_DETERMINE_IF_PERMANENT, # Assuming the worst
+ PolicyVerdict.REJECTED_PERMANENTLY,
+ }
+
class BasePolicy(object):
diff --git a/britney2/utils.py b/britney2/utils.py
index b518dd9..99f83e9 100644
--- a/britney2/utils.py
+++ b/britney2/utils.py
@@ -801,6 +801,11 @@ def invalidate_excuses(excuses, valid, invalid):
# if the dependency can be satisfied by a testing-proposed-updates excuse, skip the item
if (ename + "_tpu") in valid:
continue
+
+ rdep_verdict = PolicyVerdict.REJECTED_WAITING_FOR_ANOTHER_ITEM
+ if excuses[ename].policy_verdict.is_blocked:
+ rdep_verdict = PolicyVerdict.REJECTED_BLOCKED_BY_ANOTHER_ITEM
+
# loop on the reverse dependencies
for x in revdeps[ename]:
if x in valid:
@@ -815,8 +820,8 @@ def invalidate_excuses(excuses, valid, invalid):
invalid.append(valid.pop(p))
excuses[x].addhtml("Invalidated by dependency")
excuses[x].addreason("depends")
- if excuses[x].policy_verdict.value < PolicyVerdict.REJECTED_TEMPORARILY.value:
- excuses[x].policy_verdict = PolicyVerdict.REJECTED_TEMPORARILY
+ if excuses[x].policy_verdict.value < rdep_verdict.value:
+ excuses[x].policy_verdict = rdep_verdict
def compile_nuninst(binaries_t, inst_tester, architectures, nobreakall_arches):
--
2.11.0
Attachment:
signature.asc
Description: OpenPGP digital signature