Uploaded image for project: 'Skara'
  1. Skara
  2. SKARA-1199

Enforce maintainer approval in JBS before allowing to integrate backports into updates projects

XMLWordPrintable

    • Icon: Enhancement Enhancement
    • Resolution: Fixed
    • Icon: P4 P4
    • 1.0
    • None
    • bots
    • None

      To reduce complexity and make reviewing easier, I (Erik Joelsson) am splitting this enhancement into two parts. This issue is now only about tracking the approval labels in the bugs associated with a PR, and blocking integration until they are present. Any new way of interacting with the approval process through PRs is handled in SKARA-1949.

      Based on the discussion here: https://mail.openjdk.org/pipermail/jdk-updates-dev/2022-August/016451.html, I think we have agreed to keeping the approval process in JBS, but make Skara aware of it for a smoother user experience. When this enhancement was originally filed, there were limitations in the Skara bot architecture that made it expensive to poll bugs for updating the state of a PR. These limitations have now been solved, and having PR checks react to bug updates is close to trivial (since SKARA-1912). This means we can implement this completely inside the PullRequestBot module and will only need to adjust the configuration format of this bot.

      In addition to the OpenJDK process for maintainer approval, there are some other similar processes that use labels in JBS to allow or block integration, each with different sets of labels and naming schemes. Because of this, we need the configuration to be rather flexible. I propose a format like this (examples):

      The simple case, where the labels are the same for every branch in a repository:

      "approval": {
        "request": "jdk17u-fix-request",
        "approved": "jdk17u-fix-yes",
        "rejected": "jdk17u-fix-no",
      }

      To reduce the need for changing multiple strings when copying a configuration for a new repository, there is an optional "prefix" field:

      "approval": {
        "prefix": "jdk17u-fix-",
        "request": "request",
        "approved": "yes",
        "rejected": "no",
      }

      When there are multiple branches with different labels, having the prefix set per branch can help reduce the size of the configuration significantly:

      "approval": {
        "request": "-critical-request",
        "approved": "-critical-approved",
        "rejected": "-critical-rejected",
        "branches": [
          "jdk20\\.0\\.1": { "prefix": "CPU23_04" }
        ]
      }

      So, each of "request", "approved" and "rejected" are the base label names for the three different functions. There is an optional prefix that gets applied to all labels, and this prefix can be overridden for each branch, or for a regex expression matching a branch.

      When the "approval" configuration contains a "branches" field, then only branches that match any of those regexp elements will be subject to the approval requirement.

      In CheckRun, when listing the associated bugs, we should add the current approval state to each bug like this to make the current state easy to see:

      JDK-000000: Foo bar bug0 (Bug - P4)
      JDK-000001: Foo bar bug1 (Bug - P4 - Requested)
      JDK-000002: Foo bar bug2 (Bug - P4 - Approved)
      JDK-000003: Foo bar bug3 (Bug - P4 - Rejected)

      In botSpecificProgresses, we need to check the approval state of all associated issues and provide the checkbox line, simliar to CSR and JEP.

      When evaluating the PR metadata hash to see if CheckWorkItem needs to perform a new check, we need to add the set of labels from all associated issues.

      When a PR has been reviewed, but is still missing maintainer approval on bugs, we should add a comment in place of the "mergeReadyComment" that explains the need for maintainer approval and links to the process. This link should probably be configurable as different repos may have different processes. Something like: "This pull request has now been reviewed. Before it can be integrated, it requires additional approval." (where "additional approval" is a link)

            zsong Zhao Song
            clanger Christoph Langer
            Votes:
            0 Vote for this issue
            Watchers:
            13 Start watching this issue

              Created:
              Updated:
              Resolved: