Unverified Commit 6242b006 authored by Jörg Thalheim's avatar Jörg Thalheim Committed by GitHub
Browse files

First-class GitHub team reviews (#456341)

parents 4647cbcd 60773fe3

.github/workflows/reviewers.yml

+3 −8
Original line number Diff line number Diff line
@@ -146,19 +146,14 @@ jobs: 
      - name: Requesting maintainer reviews

        if: ${{ steps.app-token.outputs.token }}

        env:

          GH_TOKEN: ${{ github.token }}

          APP_GH_TOKEN: ${{ steps.app-token.outputs.token }}

          GH_TOKEN: ${{ steps.app-token.outputs.token }}

          REPOSITORY: ${{ github.repository }}

          ORG_ID: ${{ github.repository_owner_id }}

          NUMBER: ${{ github.event.number }}

          AUTHOR: ${{ github.event.pull_request.user.login }}

          # Don't request reviewers on draft PRs

          DRY_MODE: ${{ github.event.pull_request.draft && '1' || '' }}

        run: |

          # maintainers.json contains GitHub IDs. Look up handles to request reviews from.

          # There appears to be no API to request reviews based on GitHub IDs

          jq -r 'keys[]' comparison/maintainers.json \

            | while read -r id; do gh api /user/"$id" --jq .login; done \

            | GH_TOKEN="$APP_GH_TOKEN" result/bin/request-reviewers.sh "$REPOSITORY" "$NUMBER" "$AUTHOR"

        run: result/bin/request-maintainers-reviews.sh "$ORG_ID" "$REPOSITORY" "$NUMBER" "$AUTHOR" comparison



      - name: Log current API rate limits (app-token)

        if: ${{ steps.app-token.outputs.token }}

ci/eval/compare/default.nix

+7 −2
Original line number Diff line number Diff line
@@ -179,8 +179,12 @@ runCommand "compare" 
      jq

      cmp-stats

    ];

    maintainers = builtins.toJSON maintainers;

    passAsFile = [ "maintainers" ];

    maintainers = builtins.toJSON maintainers.users;

    teams = builtins.toJSON maintainers.teams;

    passAsFile = [

      "maintainers"

      "teams"

    ];

  }

  ''

    mkdir $out
@@ -223,4 +227,5 @@ runCommand "compare" 
    fi



    cp "$maintainersPath" "$out/maintainers.json"

    cp "$teamsPath" "$out/teams.json"

  ''

ci/eval/compare/maintainers.nix

+38 −14
Original line number Diff line number Diff line
@@ -51,15 +51,16 @@ let 
        # updates to .json files.

        # TODO: Support by-name package sets.

        filenames = lib.optional (lib.length path == 1) "pkgs/by-name/${sharded (lib.head path)}/";

        # TODO: Refactor this so we can ping entire teams instead of the individual members.

        # Note that this will require keeping track of GH team IDs in "maintainers/teams.nix".

        maintainers = package.meta.maintainers or [ ];

        # meta.maintainers also contains all individual team members.

        # We only want to ping individuals if they're added individually as maintainers, not via teams.

        maintainers = package.meta.nonTeamMaintainers or [ ];

        teams = package.meta.teams or [ ];

      }

    ))

    # No need to match up packages without maintainers with their files.

    # This also filters out attributes where `packge = null`, which is the

    # case for libintl, for example.

    (lib.filter (pkg: pkg.maintainers != [ ]))

    (lib.filter (pkg: pkg.maintainers != [ ] || pkg.teams != [ ]))

  ];



  relevantFilenames =
@@ -94,20 +95,43 @@ let 


  attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames;



  listToPing = lib.concatMap (

  userPings =

    pkg:

    map (maintainer: {

      id = maintainer.githubId;

      inherit (maintainer) github;

      type = "user";

      user = toString maintainer.githubId;

      packageName = pkg.name;

      dueToFiles = pkg.filenames;

    }) pkg.maintainers

    });



  teamPings =

    pkg: team:

    if team ? github then

      [

        {

          type = "team";

          team = toString team.githubId;

          packageName = pkg.name;

        }

      ]

    else

      userPings pkg team.members;



  maintainersToPing = lib.concatMap (

    pkg: userPings pkg pkg.maintainers ++ lib.concatMap (teamPings pkg) pkg.teams

  ) attrsWithModifiedFiles;



  byMaintainer = lib.groupBy (ping: toString ping.id) listToPing;

  byType = lib.groupBy (ping: ping.type) maintainersToPing;



  packagesPerMaintainer = lib.mapAttrs (

    maintainer: packages: map (pkg: pkg.packageName) packages

  ) byMaintainer;

  byUser = lib.pipe (byType.user or [ ]) [

    (lib.groupBy (ping: ping.user))

    (lib.mapAttrs (_user: lib.map (pkg: pkg.packageName)))

  ];

  byTeam = lib.pipe (byType.team or [ ]) [

    (lib.groupBy (ping: ping.team))

    (lib.mapAttrs (_team: lib.map (pkg: pkg.packageName)))

  ];

in

packagesPerMaintainer
 
{

  users = byUser;

  teams = byTeam;

}

ci/request-reviews/default.nix

+2 −0
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@ stdenvNoCC.mkDerivation { 
      ./get-code-owners.sh

      ./request-reviewers.sh

      ./request-code-owner-reviews.sh

      ./request-maintainers-reviews.sh

    ];

  };

  nativeBuildInputs = [ makeWrapper ];
@@ -26,6 +27,7 @@ stdenvNoCC.mkDerivation { 
    for bin in *.sh; do

      mv "$bin" "$out/bin"

      wrapProgram "$out/bin/$bin" \

        --set PAGER cat \

        --set PATH ${

          lib.makeBinPath [

            coreutils

ci/request-reviews/get-code-owners.sh

+5 −35
Original line number Diff line number Diff line 
#!/usr/bin/env bash



# Get the code owners of the files changed by a PR, returning one username per line

# Get the code owners of the files changed by a PR, returning one GitHub user/team handle per line



set -euo pipefail
@@ -29,9 +29,9 @@ log "This PR touches ${#touchedFiles[@]} files" 
# remove code owners to avoid pinging them

git -C "$gitRepo" show "$baseRef":"$ownersFile" > "$tmp"/codeowners



# Associative array with the user as the key for easy de-duplication

# Associative array with the user/team as the key for easy de-duplication

# Make sure to always lowercase keys to avoid duplicates with different casings

declare -A users=()

declare -A finalOwners=()



for file in "${touchedFiles[@]}"; do

    result=$(codeowners --file "$tmp"/codeowners "$file")
@@ -59,39 +59,9 @@ for file in "${touchedFiles[@]}"; do 
        # The first regex match is everything after the @

        entry=${BASH_REMATCH[1]}



        if [[ "$entry" =~ (.*)/(.*) ]]; then

            # Teams look like $org/$team

            org=${BASH_REMATCH[1]}

            team=${BASH_REMATCH[2]}



            # Instead of requesting a review from the team itself,

            # we request reviews from the individual users.

            # This is because once somebody from a team reviewed the PR,

            # the API doesn't expose that the team was already requested for a review,

            # so we wouldn't be able to avoid rerequesting reviews

            # without saving some some extra state somewhere



            # We could also consider implementing a more advanced heuristic

            # in the future that e.g. only pings one team member,

            # but escalates to somebody else if that member doesn't respond in time.

            gh api \

                --cache=1h \

                -H "Accept: application/vnd.github+json" \

                -H "X-GitHub-Api-Version: 2022-11-28" \

                "/orgs/$org/teams/$team/members" \

                --jq '.[].login' > "$tmp/team-members"

            readarray -t members < "$tmp/team-members"

            log "Team $entry has these members: ${members[*]}"



            for user in "${members[@]}"; do

                users[${user,,}]=

            done

        else

            # Everything else is a user

            users[${entry,,}]=

        fi

        finalOwners[${entry,,}]=

    done



done



printf "%s\n" "${!users[@]}"
 
printf "%s\n" "${!finalOwners[@]}"