From d687062cc4142e27565e9dd2f6e8e369e0f3689f Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Tue, 16 Apr 2024 15:47:13 -0700 Subject: [PATCH] Always apply obsoletion check in check_karma_thresholds The "obsolete on unstable_karma" branch being `elif`ed with the "disable autokarma on any negative karma" branch means that, if we reach the unstable_karma threshold when autokarma is enabled, we don't obsolete the update. That's clearly wrong. The most likely way to hit this is for unstable_karma to be -1, so the first negative karma causes both conditions to be met at once. But it's also possible if e.g. unstable_karma is -2, the update receives one negative karma, the maintainer re-enables autokarma, then the update receives another negative karma. We should *always* obsolete an update when it reaches unstable_karma, regardless of anything else we do. If this means we both obsolete it and disable autokarma at the same time, I don't see any problem with that, it seems correct. Signed-off-by: Adam Williamson --- bodhi-server/bodhi/server/models.py | 14 +++++---- bodhi-server/tests/services/test_updates.py | 33 +++++++++++++-------- 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/bodhi-server/bodhi/server/models.py b/bodhi-server/bodhi/server/models.py index a4acb8d52b..66efeec9b4 100644 --- a/bodhi-server/bodhi/server/models.py +++ b/bodhi-server/bodhi/server/models.py @@ -3810,6 +3810,12 @@ def check_karma_thresholds(self, db, agent): # Also return if the status of the update is not in testing and the release is frozen if self.status != UpdateStatus.testing and self.release.state == ReleaseState.frozen: return + # Obsolete if unstable karma threshold reached + if self.unstable_karma and self.karma <= self.unstable_karma: + log.info("Automatically obsoleting %s (reached unstable karma threshold)", self.alias) + self.obsolete(db) + notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict( + dict(update=self, status='unstable'))) # If an update receives negative karma disable autopush # exclude rawhide updates see #4566 if (self.autokarma or self.autotime) and self._composite_karma[1] != 0 and \ @@ -3820,7 +3826,8 @@ def check_karma_thresholds(self, db, agent): self.autotime = False text = config.get('disable_automatic_push_to_stable') self.comment(db, text, author='bodhi') - elif self.stable_karma and self.karma >= self.stable_karma \ + # If update with autopush reaches threshold, set request stable + if self.stable_karma and self.karma >= self.stable_karma \ and self.release.composed_by_bodhi and self.autokarma: # Updates for releases not "composed by Bodhi" (Rawhide, # ELN...) are pushed stable only by approve_testing.py @@ -3834,11 +3841,6 @@ def check_karma_thresholds(self, db, agent): self.set_request(db, UpdateRequest.stable, agent) notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict( dict(update=self, status='stable'))) - elif self.unstable_karma and self.karma <= self.unstable_karma: - log.info("Automatically obsoleting %s (reached unstable karma threshold)", self.alias) - self.obsolete(db) - notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict( - dict(update=self, status='unstable'))) @property def builds_json(self): diff --git a/bodhi-server/tests/services/test_updates.py b/bodhi-server/tests/services/test_updates.py index 7380a20d78..ab8b56efb9 100644 --- a/bodhi-server/tests/services/test_updates.py +++ b/bodhi-server/tests/services/test_updates.py @@ -4720,23 +4720,32 @@ def test_obsolete_if_unstable(self, autokarma, composed_by_bodhi, req, status): # caused by the obsoletion to have taken effect when we construct the # expected message up.comment(self.db, "foo", -1, 'bowlofeggs') - with fml_testing.mock_sends( - update_schemas.UpdateKarmaThresholdV1.from_dict( + if req: + # it's difficult to construct the expected messsage in this + # case because posting this comment obsoletes the update, + # publishes the UKT message, *then also* disables autopush, + # so we never quite have access to the state of the update + # right at the time the UKT message is published + expukt = update_schemas.UpdateKarmaThresholdV1 + else: + # in this case we *only* obsolete the update, we don't also + # disable autopush, so we can construct the expected message - + # the current update state is the same as the state when the + # message was published + expukt = update_schemas.UpdateKarmaThresholdV1.from_dict( {'update': up, 'status': 'unstable'} - ), - update_schemas.UpdateCommentV1 - ): + ) + with fml_testing.mock_sends(expukt, update_schemas.UpdateCommentV1): # doing a db commit causes the message to be published self.db.commit() assert up.status is UpdateStatus.obsolete assert up.request is None - # the obsolete path should not disable autokarma, but we can't assert - # this unconditionally because we might have hit the earlier disable- - # autokarma path - if ( - req is UpdateRequest.stable - or not composed_by_bodhi - ): + # the obsolete path itself should not disable autokarma, but if + # the release is composed by bodhi, we *should* also have hit + # the 'disable autokarma for any negative karma' path + if composed_by_bodhi: + assert not up.autokarma + else: assert up.autokarma is autokarma assert up.karma == -2