Skip to content

Instantly share code, notes, and snippets.

@jodh-intel
Created August 26, 2020 15:44
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save jodh-intel/8686dd0ad04c9526cb5ba93283805561 to your computer and use it in GitHub Desktop.
Save jodh-intel/8686dd0ad04c9526cb5ba93283805561 to your computer and use it in GitHub Desktop.
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)"
@c3d
Copy link

c3d commented Sep 1, 2020

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

@c3d
Copy link

c3d commented Sep 1, 2020

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

@jodh-intel
Copy link
Author

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.

@c3d
Copy link

c3d commented Sep 7, 2020

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

@ariel-adam
Copy link

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment