ci: review request maintainers

[awwpotato]

very janky and probably doesn't work

[trueNAHO]

To reduce needless maintainer pings, certain scenarios should be conservatively filtered out. For example, when a PR touches many modules, their module maintainers should not be pinged because the PR might be a simple refactor. The safest criteria is when a PR touches only a single module.

Consider taking inspiration from the heuristics I implemented in #749.

[danth]

Alternatively, put a maximum limit on the number of maintainers, so that we still allow cases where all the modules of a particular maintainer were changed.

[danth]

When this is complete we can also remove the "Maintainer CC" section from the pull request template

[awwpotato]

anyone know why this error is happening?

 error:
       … in the condition of the assert statement
         at /nix/store/g4ppspdl4fy7hnp4jgjl4ll03v7i08w3-source/pkgs/build-support/trivial-builders/default.nix:136:5:
          135|     # TODO: To fully deprecate, replace the assertion with `lib.isString` and remove the warning
          136|     assert lib.assertMsg (lib.strings.isConvertibleWithToString text) ''
             |     ^
          137|       pkgs.writeText ${lib.strings.escapeNixString name}: The second argument should be a string, but it's a ${builtins.typeOf text} instead.'';

       … in the left operand of the OR (||) operator
         at /nix/store/g4ppspdl4fy7hnp4jgjl4ll03v7i08w3-source/lib/asserts.nix:39:31:
           38|   # TODO(Profpatsch): add tests that check stderr
           39|   assertMsg = pred: msg: pred || builtins.throw msg;
             |                               ^
           40|

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: attribute 'name' missing

[danth]

Related: #1074

[awwpotato]

I don't believe that's an issue when it's pull_request, only with pull_request_target. I should probably change it to pull_request_target tho.

[danth]

Note that pull_request_target gets some extra permissions on the token, which can be risky. See the warning here.

[trueNAHO]

To avoid string injections, we should use environment variables:

env:
  GH_REPO: ${{ github.repository }}
  PR_AUTHOR: ${{ github.event.pull_request.user.login }}
  PR_NUMBER: ${{ github.event.pull_request.number }}

The Bash code could be polished to something like the following partially (not on GitHub) tested:

MAX_MAINTAINERS=5

existing_reviewers="$(
  curl \
    --silent \
    "https://api.github.com/repos/$GH_REPO/pulls/$PR_NUMBER/requested_reviewers" |
    jq --raw-output [.users[].login]
)"

to_ping=()

jq --raw-output '.[]' <<<"$CHANGED_FILES" | while read -r file; do
  jq --arg file "$file" --raw-output '.[$file][]' result/gh_ids.json |
    while read -r gh_id;
  do
    # Do not request the original author or already requested reviewers.
    if [[ "$gh_id" == "$PR_AUTHOR" || "$existing_reviewers" =~ $gh_id ]]; then
        continue
    fi

    to_ping+=("$gh_id")
  done
done

if ((${#to_ping[@]} > 0 && ${#to_ping[@]} <= MAX_MAINTAINERS)); then
  printf '%s\n' "::set-output name=to-ping::$(printf '%s,' "${to_ping[@]}")"
fi

The current "$existing_reviewers" =~ $gh_id check is fragile and should be replaced with a jq command.