Overhaul various 'update status' checks - meets_testing_requirements, check_requirements, critpath_approved, check_karma_thresholds - and approve_testing script

bodhi-server/bodhi/server/config.py

@@ -360,13 +360,7 @@ class BodhiConfig(dict):
         'critpath.jsonpath': {
             'value': '/etc/bodhi/critpath',
             'validator': str},
-        'critpath.min_karma': {
-            'value': 2,
-            'validator': int},
-        'critpath.num_admin_approvals': {
-            'value': 2,
-            'validator': int},
-        'critpath.stable_after_days_without_negative_karma': {
+        'critpath.mandatory_days_in_testing': {
             'value': 14,
             'validator': int},
         'critpath.type': {
@@ -452,6 +446,9 @@ class BodhiConfig(dict):
         'mako.directories': {
             'value': 'bodhi.server:templates',
             'validator': str},
+        'min_karma': {
+            'value': 2,
+            'validator': int},
         'mandatory_packager_groups': {
             'value': ['packager'],
             'validator': _generate_list_validator()},

bodhi-server/bodhi/server/services/updates.py

@@ -211,21 +211,10 @@ def set_request(request):
                            "Pushing back to testing a stable update is not allowed")
         return
 
-    if action == UpdateRequest.stable:
-        settings = request.registry.settings
-        result, reason = update.check_requirements(request.db, settings)
-        if not result:
-            log.info(
-                f'Unable to set request for {update.alias} to stable due to failed requirements: '
-                f'{reason}')
-            request.errors.add('body', 'request',
-                               'Requirement not met %s' % reason)
-            return
-
     try:
         update.set_request(request.db, action, request.identity.name)
     except BodhiException as e:
-        log.info("Failed to set the request: %s", e)
+        log.info(f"Failed to set the request: {e}")
         request.errors.add('body', 'request', str(e))
     except Exception as e:
         log.exception("Unhandled exception in set_request")

bodhi-server/bodhi/server/tasks/approve_testing.py

@@ -15,11 +15,30 @@
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-"""Comment on updates after they reach the mandatory amount of time in the testing repository."""
 
-import logging
+"""
+Make appropriate changes to updates that reach certain karma or time-in-stable thresholds.
+
+This script is intended to run as a regular job (cronjob or the like). It finds all updates in
+UpdateStatus.testing with no request, then does one of three things. If the update does not meet
+testing requirements (i.e. it doesn't have enough karma or time in testing to even be manually
+pushed, or it fails gating checks), we do nothing. Otherwise, we set the date_approved
+for the update if it's not already set, then choose one of the two other options.
+
+If the update meets testing requirements, has autopush after a certain time in testing (autotime)
+enabled, and has reached that threshold, we push it. Note this is the **ONLY** way updates
+for releases "not composed by Bodhi" (Rawhide, ELN, Branched for the first few weeks) are ever
+pushed stable. The other way updates can be autopushed stable - Update.check_karma_thresholds(),
+called by Update.comment() - opts out of handling "not composed by Bodhi" release updates.
 
-from sqlalchemy import func
+If the update meets testing requirements but does not have autotime enabled or has
+not reached that threshold, we check to see if the update already has a comment saying it is
+now "approved" for push to stable. If not, we post that comment, and publish the
+UpdateRequirementsMetStableV1 message.
+"""
+
+import datetime
+import logging
 
 from bodhi.messages.schemas import update as update_schemas
 from bodhi.server import Session, notifications, buildsys
@@ -32,136 +51,163 @@
 
 
 def main():
-    """
-    Comment on updates that are eligible to be pushed to stable.
-
-    Queries for updates in the testing state that have a NULL request, and run approve_update on
-    them.
-    """
+    """Query for updates in the testing state that have a NULL request, and run process_update."""
     db_factory = transactional_session_maker()
     try:
         with db_factory() as db:
             testing = db.query(Update).filter_by(status=UpdateStatus.testing, request=None)
             for update in testing:
-                approve_update(update, db)
+                process_update(update, db)
                 db.commit()
     except Exception:
         log.exception("There was an error approving testing updates.")
     finally:
         db_factory._end_session()
 
 
-def approve_update(update: Update, db: Session):
-    """Add a comment to an update if it is ready for stable.
+def autopush_update(update: Update, db: Session):
+    """
+    Push an update that has autopush for time enabled and has reached the required threshold.
+
+    For releases composed by Bodhi, we set the request and leave the compose process to do the
+    status change. For releases not composed by Bodhi, we do the status change here.
+    """
+    if not update.has_stable_comment:
+        notifications.publish(update_schemas.UpdateRequirementsMetStableV1.from_dict(
+            dict(update=update)))
+    log.info(f"Automatically marking {update.alias} as stable")
+    # For releases composed by Bodhi, just set the request, and leave
+    # the rest to the composer
+    if update.release.composed_by_bodhi:
+        update.set_request(db=db, action=UpdateRequest.stable, username="bodhi")
+        return
+    # For releases not composed by Bodhi, do all the work here
+    # Both side-tag and non-side-tag updates
+    conflicting_builds = update.find_conflicting_builds()
+    if conflicting_builds:
+        builds_str = str.join(", ", conflicting_builds)
+        update.comment(
+            db,
+            "This update cannot be pushed to stable. "
+            f"These builds {builds_str} have a more recent "
+            f"build in koji's {update.release.stable_tag} tag.",
+            author="bodhi")
+        update.request = None
+        if update.from_tag is not None:
+            update.status = UpdateStatus.pending
+            update.remove_tag(
+                update.release.get_pending_testing_side_tag(update.from_tag))
+        else:
+            update.status = UpdateStatus.obsolete
+            update.remove_tag(update.release.pending_testing_tag)
+            update.remove_tag(update.release.candidate_tag)
+        db.commit()
+        log.info(f"{update.alias} has conflicting builds - bailing")
+        return
+    update.add_tag(update.release.stable_tag)
+    update.status = UpdateStatus.stable
+    update.request = None
+    update.pushed = True
+    update.date_stable = datetime.datetime.utcnow()
+    update.comment(db, "This update has been submitted for stable by bodhi",
+                   author=u'bodhi')
+    update.modify_bugs()
+    db.commit()
+    if update.from_tag:
+        # Merging the side tag should happen here
+        pending_signing_tag = update.release.get_pending_signing_side_tag(
+            update.from_tag)
+        testing_tag = update.release.get_pending_testing_side_tag(update.from_tag)
+        update.remove_tag(pending_signing_tag)
+        update.remove_tag(testing_tag)
+        update.remove_tag(update.from_tag)
+        # Delete side-tag and its children after Update has enter stable
+        # We can't fully rely on Koji's auto-purge-when-empty because
+        # there may be older nvrs tagged in the side-tag
+        koji = buildsys.get_session()
+        koji.multicall = True
+        koji.deleteTag(pending_signing_tag)
+        koji.deleteTag(testing_tag)
+        koji.deleteTag(update.from_tag)
+        koji.multiCall()
+    else:
+        # Non side-tag updates
+        update.remove_tag(update.release.pending_testing_tag)
+        update.remove_tag(update.release.pending_stable_tag)
+        update.remove_tag(update.release.pending_signing_tag)
+        update.remove_tag(update.release.testing_tag)
+        update.remove_tag(update.release.candidate_tag)
+
+
+def approved_comment_message(update: Update, db: Session):
+    """Post "approved" comment and publish UpdatesRequirementsMetStable message."""
+    # If this update was already commented, skip it
+    if update.has_stable_comment:
+        log.info(f"{update.alias} has already the comment that it can be pushed to stable - "
+                 "bailing")
+        return
+    # post the comment
+    update.comment(
+        db,
+        str(config.get('testing_approval_msg')),
+        author='bodhi',
+        # Only send email notification about the update reaching
+        # testing approval on releases composed by bodhi
+        email_notification=update.release.composed_by_bodhi
+    )
+    # publish the message
+    notifications.publish(update_schemas.UpdateRequirementsMetStableV1.from_dict(
+        dict(update=update)))
+
+
+def process_update(update: Update, db: Session):
+    """
+    Check requirements, update date_approved, then call appropriate handler function.
+
+    In all cases, this will set date_approved if the update "meets testing requirements" -
+    which means it has reached either the minimum karma or time threshold to be pushed
+    stable, and its gating status is passed - and that date has not been set before.
+
+    After that, if the update has automatic push for time enabled and has reached the required
+    time threshold, this will call autopush_update to handle pushing it. It is intentional
+    that we do not called approved_comment_message() in this case - there is no point alerting
+    the maintainer that the update can be pushed manually if we are already going to push it
+    automatically. See issue #3846.
+
+    Otherwise - if the update is approved but is not being autopushed for time - this will
+    call approved_comment_message() to post the approval comment and publish the
+    UpdateRequirementsMetStable message, if it has not been done before.
 
-    Check that the update is eligible to be pushed to stable but hasn't had comments from Bodhi to
-    this effect. Add a comment stating that the update may now be pushed to stable.
+    It has not yet proven necessary to check the karma autopush threshold here. For releases that
+    are "composed by Bodhi", Update.check_karma_thresholds() pushes updates as soon as they
+    reach the karma autopush threshold. For releases that are not "composed by Bodhi", on update
+    creation, autotime is forced to True and the time threshold is forced to 0, thus updates for
+    these releases are *always* eligible for autotime push here, as soon as they pass gating,
+    there is no case in which autokarma would be relevant.
 
     Args:
         update: an update in testing that may be ready for stable.
+        db: a database session.
     """
-    if not update.release.mandatory_days_in_testing and not update.autotime:
-        # If this release does not have any testing requirements and is not autotime,
-        # skip it
-        log.info(f"{update.alias} doesn't have mandatory days in testing - bailing")
-        return
-    # If updates have reached the testing threshold, say something! Keep in mind
-    # that we don't care about karma here, because autokarma updates get their request set
-    # to stable by the Update.comment() workflow when they hit the required threshold. Thus,
-    # this function only needs to consider the time requirements because these updates have
-    # not reached the karma threshold.
+    # meets_testing_requirements will be True if all non-karma / non-time
+    # requirements are met, and the update has reached the minimum karma
+    # threshold or wait period for a manual push to be allowed, so this
+    # means "update is eligible to be manually pushed stable"
     if not update.meets_testing_requirements:
         log.info(f"{update.alias} has not met testing requirements - bailing")
         return
     log.info(f'{update.alias} now meets testing requirements')
-    # If the update is going to be pushed automatically to stable, do not
-    # double comment that the maintainer can push it manually (#3846)
-    if not update.autotime or update.days_in_testing < update.stable_days:
-        # If this update was already commented, skip it
-        if update.has_stable_comment:
-            log.info(f"{update.alias} has already the comment that it can be pushed to stable - "
-                     "bailing")
-            return
-        # Only send email notification about the update reaching
-        # testing approval on releases composed by bodhi
-        update.comment(
-            db,
-            str(config.get('testing_approval_msg')),
-            author='bodhi',
-            email_notification=update.release.composed_by_bodhi
-        )
-        notifications.publish(update_schemas.UpdateRequirementsMetStableV1.from_dict(
-            dict(update=update)))
+    # always set date_approved, if it has never been set before: this
+    # date indicates "first date update became eligible for manual push"
+    if not update.date_approved:
+        update.date_approved = datetime.datetime.utcnow()
     if update.autotime and update.days_in_testing >= update.stable_days:
-        if not update.has_stable_comment:
-            notifications.publish(update_schemas.UpdateRequirementsMetStableV1.from_dict(
-                dict(update=update)))
-        log.info(f"Automatically marking {update.alias} as stable")
-        # For now only rawhide update can be created using side tag
-        # Do not add the release.pending_stable_tag if the update
-        # was created from a side tag.
-        if update.release.composed_by_bodhi:
-            if not update.date_approved:
-                update.date_approved = func.current_timestamp()
-            update.set_request(db=db, action=UpdateRequest.stable, username="bodhi")
-        # For updates that are not included in composes run by bodhi itself,
-        # mark them as stable
-        else:
-            # Single and Multi build update
-            conflicting_builds = update.find_conflicting_builds()
-            if conflicting_builds:
-                builds_str = str.join(", ", conflicting_builds)
-                update.comment(
-                    db,
-                    "This update cannot be pushed to stable. "
-                    f"These builds {builds_str} have a more recent "
-                    f"build in koji's {update.release.stable_tag} tag.",
-                    author="bodhi")
-                update.request = None
-                if update.from_tag is not None:
-                    update.status = UpdateStatus.pending
-                    update.remove_tag(
-                        update.release.get_pending_testing_side_tag(update.from_tag))
-                else:
-                    update.status = UpdateStatus.obsolete
-                    update.remove_tag(update.release.pending_testing_tag)
-                    update.remove_tag(update.release.candidate_tag)
-                db.commit()
-                log.info(f"{update.alias} has conflicting builds - bailing")
-                return
-            update.add_tag(update.release.stable_tag)
-            update.status = UpdateStatus.stable
-            update.request = None
-            update.pushed = True
-            update.date_stable = update.date_approved = func.current_timestamp()
-            update.comment(db, "This update has been submitted for stable by bodhi",
-                           author=u'bodhi')
-            update.modify_bugs()
-            db.commit()
-            # Multi build update
-            if update.from_tag:
-                # Merging the side tag should happen here
-                pending_signing_tag = update.release.get_pending_signing_side_tag(
-                    update.from_tag)
-                testing_tag = update.release.get_pending_testing_side_tag(update.from_tag)
-                update.remove_tag(pending_signing_tag)
-                update.remove_tag(testing_tag)
-                update.remove_tag(update.from_tag)
-                # Delete side-tag and its children after Update has enter stable
-                # We can't fully rely on Koji's auto-purge-when-empty because
-                # there may be older nvrs tagged in the side-tag
-                koji = buildsys.get_session()
-                koji.multicall = True
-                koji.deleteTag(pending_signing_tag)
-                koji.deleteTag(testing_tag)
-                koji.deleteTag(update.from_tag)
-                koji.multiCall()
-            else:
-                # Single build update
-                update.remove_tag(update.release.pending_testing_tag)
-                update.remove_tag(update.release.pending_stable_tag)
-                update.remove_tag(update.release.pending_signing_tag)
-                update.remove_tag(update.release.testing_tag)
-                update.remove_tag(update.release.candidate_tag)
+        # if update *additionally* meets the time-based autopush threshold,
+        # push it
+        autopush_update(update, db)
+    else:
+        # otherwise, post the comment and publish the message announcing
+        # it is eligible for manual push, if this has not been done
+        approved_comment_message(update, db)
 
     log.info(f'{update.alias} processed by approve_testing')

bodhi-server/bodhi/server/tasks/composer.py

@@ -469,7 +469,7 @@ def perform_gating(self):
         """Eject Updates that don't meet testing requirements from the compose."""
         log.debug('Performing gating.')
         for update in self.compose.updates:
-            result, reason = update.check_requirements(self.db, config)
+            result, reason = update.meets_requirements_why
             if not result:
                 log.warning("%s failed gating: %s" % (update.alias, reason))
                 self.eject_from_compose(update, reason)

bodhi-server/bodhi/server/templates/new_update.html

@@ -318,7 +318,7 @@ <h6 class="fw-bold">Name</h6>
           <div class="card-body pt-0">
               <div class="row ms-3">
                   <div class="col-6">
-                      <h6 class="fw-bold" title="This is the threshold of positive karma required to automatically push an update from testing to stable.">Stable Karma</h6>
+                      <h6 class="fw-bold" title="This is the threshold of positive karma required to automatically push an update from testing to stable. This happens only if 'Auto-request stable based on karma?' is checked.">Stable Karma</h6>
                       <input type="number" class="form-control" name="stable_karma" min="1" required
                       % if update:
                       value="${update.stable_karma}"
@@ -328,7 +328,7 @@ <h6 class="fw-bold" title="This is the threshold of positive karma required to a
                     />
                   </div>
                   <div class="col-6">
-                      <h6 class="fw-bold" title="If checked, this option allows bodhi to automatically move your update from testing to stable once enough time was spent in testing.">Unstable Karma</h6>
+                      <h6 class="fw-bold" title="This is the threshold of negative karma required to automatically unpush an update. This will happen whether or not 'Auto-request stable based on karma?' is checked.">Unstable Karma</h6>
                       <input type="number" class="form-control" name="unstable_karma" max="-1" required
                       % if update:
                       value="${update.unstable_karma}"