Unverified Commit f7d6d11e authored by Wolfgang Walther's avatar Wolfgang Walther
Browse files

workflows/check: don't check github api for owners file 

This removes the "owners" check from codeowners-validator. With it, all
tokens and permissions can be removed, because these were only needed to
make these requests.

This solves the problem of codeowners-validator not supporting our new
nested team structure for nixpkgs-maintainers. To make the onboarding of
new teams easier, we moved all teams "under" the nixpkgs-maintainers
team. This makes them inherit the right privileges (triage) for Nixpkgs.

However, this inheritance is not recognized by codeowners-validator,
thus it assumes that these teams don't have access to Nixpkgs. This then
fails the owners check immediately.

Removing the owners check also has a few other advantages:
- This check depends on external state: If a user is renamed or a team
removed, the check will fail. This makes it a bad check for required
status checks or merge queues - the check might fail randomly,
independent of the current PR.
- Running this check in a fork will never work, because the respective
users and teams don't have access to the fork's repo.

Both of this required us to set `continue-on-error: true` most of the
time.
parent 8353fec8

.github/workflows/check.yml

+4 −42
Original line number Diff line number Diff line
@@ -12,17 +12,12 @@ on: 
      mergedSha:

        required: true

        type: string

      ownersCanFail:

        required: true

        type: boolean

      targetSha:

        required: true

        type: string

    secrets:

      CACHIX_AUTH_TOKEN:

        required: true

      OWNER_RO_APP_PRIVATE_KEY:

        required: true



permissions: {}
@@ -72,19 +67,8 @@ jobs: 
          GH_TOKEN: ${{ github.token }}

        run: gh api /rate_limit | jq



  # For checking code owners, this job depends on a GitHub App with the following permissions:

  # - Permissions:

  #   - Repository > Administration: read-only

  #   - Organization > Members: read-only

  # - Install App on this repository, setting these variables:

  #   - OWNER_RO_APP_ID (variable)

  #   - OWNER_RO_APP_PRIVATE_KEY (secret)

  #

  # This should not use the same app as the job to request reviewers, because this job requires

  # handling untrusted PR input.

  owners:

    runs-on: ubuntu-24.04-arm

    continue-on-error: ${{ inputs.ownersCanFail }}

    timeout-minutes: 5

    steps:

      - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
@@ -94,7 +78,6 @@ jobs: 
        uses: ./.github/actions/checkout

        with:

          merged-as-untrusted-at: ${{ inputs.mergedSha }}

          target-as-trusted-at: ${{ inputs.targetSha }}



      - uses: cachix/install-nix-action@a809471b5c7c913aa67bec8f459a11a0decc3fce # v31
@@ -107,36 +90,15 @@ jobs: 
          pushFilter: -source$



      - name: Build codeowners validator

        run: nix-build nixpkgs/trusted/ci --arg nixpkgs ./nixpkgs/trusted-pinned -A codeownersValidator



      - uses: actions/create-github-app-token@67018539274d69449ef7c02e8e71183d1719ab42 # v2.1.4

        if: github.event_name == 'pull_request_target' && vars.OWNER_RO_APP_ID

        id: app-token

        with:

          app-id: ${{ vars.OWNER_RO_APP_ID }}

          private-key: ${{ secrets.OWNER_RO_APP_PRIVATE_KEY }}

          permission-administration: read

          permission-members: read



      - name: Log current API rate limits

        if: steps.app-token.outputs.token

        env:

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

        run: gh api /rate_limit | jq

        run: nix-build nixpkgs/untrusted/ci --arg nixpkgs ./nixpkgs/untrusted-pinned -A codeownersValidator



      - name: Validate codeowners

        if: steps.app-token.outputs.token

        env:

          OWNERS_FILE: nixpkgs/untrusted/ci/OWNERS

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

          REPOSITORY_PATH: nixpkgs/untrusted

          OWNER_CHECKER_REPOSITORY: ${{ github.repository }}

          # Omits "owners", which checks whether GitHub handles exist, but fails with nested team

          # structures.

          CHECKS: "duppatterns,files,syntax"

          # Set this to "notowned,avoid-shadowing" to check that all files are owned by somebody

          EXPERIMENTAL_CHECKS: "avoid-shadowing"

        run: result/bin/codeowners-validator
 


      - name: Log current API rate limits

        if: steps.app-token.outputs.token

        env:

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

        run: gh api /rate_limit | jq

.github/workflows/pr.yml

+0 −4
Original line number Diff line number Diff line
@@ -11,8 +11,6 @@ on: 
      OWNER_APP_PRIVATE_KEY:

        # The Test workflow should not actually request reviews from owners.

        required: false

      OWNER_RO_APP_PRIVATE_KEY:

        required: true



concurrency:

  group: pr-${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }}
@@ -59,13 +57,11 @@ jobs: 
      pull-requests: write

    secrets:

      CACHIX_AUTH_TOKEN: ${{ secrets.CACHIX_AUTH_TOKEN }}

      OWNER_RO_APP_PRIVATE_KEY: ${{ secrets.OWNER_RO_APP_PRIVATE_KEY }}

    with:

      baseBranch: ${{ needs.prepare.outputs.baseBranch }}

      headBranch: ${{ needs.prepare.outputs.headBranch }}

      mergedSha: ${{ needs.prepare.outputs.mergedSha }}

      targetSha: ${{ needs.prepare.outputs.targetSha }}

      ownersCanFail: ${{ !contains(fromJSON(needs.prepare.outputs.touched), 'owners') }}



  lint:

    name: Lint

.github/workflows/test.yml

+0 −1
Original line number Diff line number Diff line
@@ -98,7 +98,6 @@ jobs: 
    secrets:

      CACHIX_AUTH_TOKEN: ${{ secrets.CACHIX_AUTH_TOKEN }}

      NIXPKGS_CI_APP_PRIVATE_KEY: ${{ secrets.NIXPKGS_CI_APP_PRIVATE_KEY }}

      OWNER_RO_APP_PRIVATE_KEY: ${{ secrets.OWNER_RO_APP_PRIVATE_KEY }}



  push:

    if: needs.prepare.outputs.push

ci/github-script/prepare.js

+0 −1
Original line number Diff line number Diff line
@@ -221,7 +221,6 @@ module.exports = async ({ github, context, core, dry }) => { 


    const touched = []

    if (files.includes('ci/pinned.json')) touched.push('pinned')

    if (files.includes('ci/OWNERS')) touched.push('owners')

    core.setOutput('touched', touched)



    return