pr-require-porting-labels.yaml

name: Ensure all PRs have the required porting labels

on:
  pull_request:
    types:
      - closed

jobs:
  check-pr-porting-labels:
    runs-on: ubuntu-latest
    steps:
      - name: Install hub
        run: |
          HUB_ARCH="amd64"
          HUB_VER=$(curl -sL "https://api.github.com/repos/github/hub/releases/latest" |\
            jq -r .tag_name | sed 's/^v//')
          curl -sL \
            "https://github.com/github/hub/releases/download/v${HUB_VER}/hub-linux-${HUB_ARCH}-${HUB_VER}.tgz" |\
          tar xz --strip-components=2 --wildcards '*/bin/hub' && \
          sudo install hub /usr/local/bin

      - name: Stop PR being merged unless it has atleast one backport label and one forward port label
        env:
          GITHUB_TOKEN: ${{ secrets.KATA_GITHUB_ACTIONS_TOKEN }}
          DEBUG: true
        run: |
          pr=${{ github.event.number }}

          # Every PR must have two porting labels before it can be merged.
          # This is to ensure it is clear that a reviewer has considered both
          # porting directions.
          backport_labels=("needs-backport" "no-backport-needed")
          forward_port_labels=("needs-forward-port" "no-forward-port-needed")

          pr_details=$(hub pr list -f '%I;%L%n' | grep "^${pr}" || true)

          [ -z "$pr_details" ] \
            && echo "::error::Cannot determine details for PR $pr" \
            && exit 1

          labels=$(echo "$pr_details"|cut -d';' -f2|tr ',' ' ')

          [ -z "$labels" ] \
            && echo "::error::PR $pr does not have required porting labels" \
            && exit 1

          backport_label=""
          for label in ${backport_labels[@]}
          do
            echo "$labels"|grep -qw "$label" \
              && backport_label="$label" \
              && break
          done

          [ -z "$backport_label" ] \
            && echo "::error::PR $pr missing a backport label (${backport_labels[@]})" \
            && exit 1

          forward_port_label=""
          for label in ${forward_port_labels[@]}
          do
            echo "$labels"|grep -qw "$label" \
              && forward_port_label="$label" \
              && break
          done

          [ -z "$forward_port_label" ] \
            && echo "::error::PR $pr missing a forward port label (${backport_labels[@]})" \
            && exit 1

          echo "::debug::PR $pr has required porting labels ($backport_label, $forward_port_label)"

Nit: && on line 39 or 45 or 52 assumes that the line above executed successfully.

What about if the PR is a backport or a forward port?

What about if the PR is a backport or a forward port?

@c3d - If a PR is a backport / forward-port PR, with the current set of labels it would presumably be marked no-backport-needed / no-forward-port-needed (since the new labels apply to a future PR, not a current back/forward port PR). However, we could add more labels (backport-change and forward-port-change). That would be useful as we could filter by these labels too. What do we think? Too many labels already? Any other way to handle this scenario?

/cc @gnawux, @amshinde, @ariel-adam.

For more details, see:

• kata-containers/kata-containers#634
• http://lists.katacontainers.io/pipermail/kata-dev/2020-August/001468.html

What about if the PR is a backport or a forward port?

@c3d - If a PR is a backport / forward-port PR, with the current set of labels it would presumably be marked no-backport-needed / no-forward-port-needed (since the new labels apply to a future PR, not a current back/forward port PR).

It's the "presumably" that is not obvious. Even with your documentation.

However, we could add more labels (backport-change and forward-port-change). That would be useful as we could filter by these labels too. What do we think? Too many labels already? Any other way to handle this scenario?

I personally slightly favor having a dedicated label, less chance of being confused.

Another possibility is to add a specific tag, e.g. forward-port-of (similar to fixes) and check that, with the additional benefit that it gives us a pointer to what we are forward-porting. This seems particularly relevant for agent code, where the connexion will not be obvious between Rust and Go commits.

/cc @gnawux, @amshinde, @ariel-adam.

I personally prefer to have a specif i

I think that although we are trying to reduce the amount of labels, in this case since there is a script enforcing their existence it does make sense.
As long as the error messages are clear enough and it's document it should work.

I would however explain in the error message what is required from the developer to fix it (and not just read the XXXXXXX manual :-)).
So for example, in "PR $pr does not have required porting labels" I'd also point out what labels should be added for it to actually pass (backport / forward-port/no-backport-needed / no-forward-port-needed)