From d7a676d0741729bb643e0b8c54b989cb747c6a4b Mon Sep 17 00:00:00 2001 From: Niels Thykier Date: Wed, 8 Feb 2017 19:01:41 +0000 Subject: [PATCH] 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 --- 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 = "%s (%s to %s)\n