From 6346ef5210cd1aedb63c1499cf1d1a3ec0d85a5d Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Wed, 18 Jun 2025 18:03:01 +0200 Subject: [PATCH] workflows/labels: manage labels with a single API call Instead of deleting each label separately and then making another call to add new labels, this replaces all labels at once, thus saving API calls in some cases. Also, the labels are now managed in object-style compared to the array-style before. This allows putting all the knowledge about each label into a single place instead of in multiple places. For example, the rebuild labels had to be special cased in the workflow before - and the nix code to compare had to match that. Also, the approval labels had to be considered in the `before` and `after` phases. The next commit shows how easy it is to add a new label now. (cherry picked from commit 5b5b18c875ca84ec3c3869d3995c9a8d80279642) --- .github/workflows/labels.yml | 72 ++++++++++++++++++------------------ ci/eval/compare/default.nix | 36 +++++++++--------- ci/eval/compare/utils.nix | 67 +++++++++++++-------------------- 3 files changed, 81 insertions(+), 94 deletions(-) diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index 3e44f8cdb0a6..870e3d98305d 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -194,19 +194,15 @@ jobs: expectedHash: artifact.digest }) - // Get all currently set labels that we manage - const before = + // Create a map (Label -> Boolean) of all currently set labels. + // Each label is set to True and can be disabled later. + const before = Object.fromEntries( (await github.paginate(github.rest.issues.listLabelsOnIssue, { ...context.repo, issue_number: pull_request.number })) - .map(({ name }) => name) - .filter(name => - name.startsWith('10.rebuild') || - name == '11.by: package-maintainer' || - name.startsWith('12.approvals:') || - name == '12.approved-by: package-maintainer' - ) + .map(({ name }) => [name, true]) + ) const approvals = new Set( (await github.paginate(github.rest.pulls.listReviews, { @@ -221,35 +217,41 @@ jobs: JSON.parse(await readFile(`${pull_request.number}/maintainers.json`, 'utf-8')) ).map(m => Number.parseInt(m, 10))) - // And the labels that should be there - const after = JSON.parse(await readFile(`${pull_request.number}/changed-paths.json`, 'utf-8')).labels - if (approvals.size > 0) after.push(`12.approvals: ${approvals.size > 2 ? '3+' : approvals.size}`) - if (Array.from(maintainers).some(m => approvals.has(m))) after.push('12.approved-by: package-maintainer') + const evalLabels = JSON.parse(await readFile(`${pull_request.number}/changed-paths.json`, 'utf-8')).labels - if (context.eventName == 'pull_request') { - core.info('Skipping labeling on a pull_request event (no privileges).') - return - } - - // Remove the ones not needed anymore - await Promise.all( - before.filter(name => !after.includes(name)) - .map(name => github.rest.issues.removeLabel({ - ...context.repo, - issue_number: pull_request.number, - name - })) + // Manage the labels + const after = Object.assign( + {}, + before, + // Ignore `evalLabels` if it's an array. + // This can happen for older eval runs, before we switched to objects. + // The old eval labels would have been set by the eval run, + // so now they'll be present in `before`. + // TODO: Simplify once old eval results have expired (~2025-10) + (Array.isArray(evalLabels) ? undefined : evalLabels), + { + '12.approvals: 1': approvals.size == 1, + '12.approvals: 2': approvals.size == 2, + '12.approvals: 3+': approvals.size >= 3, + '12.approved-by: package-maintainer': Array.from(maintainers).some(m => approvals.has(m)), + } ) - // And add the ones that aren't set already - const added = after.filter(name => !before.includes(name)) - if (added.length > 0) { - await github.rest.issues.addLabels({ - ...context.repo, - issue_number: pull_request.number, - labels: added - }) - } + // No need for an API request, if all labels are the same. + const hasChanges = Object.keys(after).some(name => (before[name] ?? false) != after[name]) + if (log('Has changes', hasChanges, !hasChanges)) + return; + + // Skipping labeling on a pull_request event, because we have no privileges. + const labels = Object.entries(after).filter(([,value]) => value).map(([name]) => name) + if (log('Set labels', labels, context.eventName == 'pull_request')) + return; + + await github.rest.issues.setLabels({ + ...context.repo, + issue_number: pull_request.number, + labels + }) } catch (cause) { throw new Error(`Labeling PR #${pull_request.number} failed.`, { cause }) } diff --git a/ci/eval/compare/default.nix b/ci/eval/compare/default.nix index 302a2df90611..46507a492a59 100644 --- a/ci/eval/compare/default.nix +++ b/ci/eval/compare/default.nix @@ -31,10 +31,10 @@ let changed: ["package2", "package3"], removed: ["package4"], }, - labels: [ - "10.rebuild-darwin: 1-10", - "10.rebuild-linux: 1-10" - ], + labels: { + "10.rebuild-darwin: 1-10": true, + "10.rebuild-linux: 1-10": true + }, rebuildsByKernel: { darwin: ["package1", "package2"], linux: ["package1", "package2", "package3"] @@ -97,19 +97,21 @@ let rebuildCountByKernel ; labels = - (getLabels rebuildCountByKernel) - # Adds "10.rebuild-*-stdenv" label if the "stdenv" attribute was changed - ++ lib.mapAttrsToList (kernel: _: "10.rebuild-${kernel}-stdenv") ( - lib.filterAttrs (_: lib.elem "stdenv") rebuildsByKernel - ) - # Adds the "11.by: package-maintainer" label if all of the packages directly - # changed are maintained by the PR's author. (https://github.com/NixOS/ofborg/blob/df400f44502d4a4a80fa283d33f2e55a4e43ee90/ofborg/src/tagger.rs#L83-L88) - ++ lib.optional ( - maintainers ? ${githubAuthorId} - && lib.all (lib.flip lib.elem maintainers.${githubAuthorId}) ( - lib.flatten (lib.attrValues maintainers) - ) - ) "11.by: package-maintainer"; + getLabels rebuildCountByKernel + # Sets "10.rebuild-*-stdenv" label to whether the "stdenv" attribute was changed. + // lib.mapAttrs' ( + kernel: rebuilds: lib.nameValuePair "10.rebuild-${kernel}-stdenv" (lib.elem "stdenv" rebuilds) + ) rebuildsByKernel + # Set the "11.by: package-maintainer" label to whether all packages directly + # changed are maintained by the PR's author. + # (https://github.com/NixOS/ofborg/blob/df400f44502d4a4a80fa283d33f2e55a4e43ee90/ofborg/src/tagger.rs#L83-L88) + // { + "11.by: package-maintainer" = + maintainers ? ${githubAuthorId} + && lib.all (lib.flip lib.elem maintainers.${githubAuthorId}) ( + lib.flatten (lib.attrValues maintainers) + ); + }; } ); diff --git a/ci/eval/compare/utils.nix b/ci/eval/compare/utils.nix index 064d2cf57ea1..d34299ca4654 100644 --- a/ci/eval/compare/utils.nix +++ b/ci/eval/compare/utils.nix @@ -151,7 +151,7 @@ rec { lib.genAttrs [ "linux" "darwin" ] filterKernel; /* - Maps an attrs of `kernel - rebuild counts` mappings to a list of labels + Maps an attrs of `kernel - rebuild counts` mappings to an attrs of labels Turns { @@ -159,54 +159,37 @@ rec { darwin = 1; } into - [ - "10.rebuild-darwin: 1" - "10.rebuild-darwin: 1-10" - "10.rebuild-linux: 11-100" - ] + { + "10.rebuild-darwin: 1" = true; + "10.rebuild-darwin: 1-10" = true; + "10.rebuild-darwin: 11-100" = false; + # [...] + "10.rebuild-darwin: 1" = false; + "10.rebuild-darwin: 1-10" = false; + "10.rebuild-linux: 11-100" = true; + # [...] + } */ getLabels = rebuildCountByKernel: - lib.concatLists ( + lib.mergeAttrsList ( lib.mapAttrsToList ( kernel: rebuildCount: let - numbers = - if rebuildCount == 0 then - [ "0" ] - else if rebuildCount == 1 then - [ - "1" - "1-10" - ] - else if rebuildCount <= 10 then - [ "1-10" ] - else if rebuildCount <= 100 then - [ "11-100" ] - else if rebuildCount <= 500 then - [ "101-500" ] - else if rebuildCount <= 1000 then - [ - "501-1000" - "501+" - ] - else if rebuildCount <= 2500 then - [ - "1001-2500" - "501+" - ] - else if rebuildCount <= 5000 then - [ - "2501-5000" - "501+" - ] - else - [ - "5001+" - "501+" - ]; + range = from: to: from <= rebuildCount && (rebuildCount <= to || to == null); in - lib.forEach numbers (number: "10.rebuild-${kernel}: ${number}") + lib.mapAttrs' (number: lib.nameValuePair "10.rebuild-${kernel}: ${number}") { + "0" = range 0 0; + "1" = range 1 1; + "1-10" = range 1 10; + "11-100" = range 11 100; + "101-500" = range 101 500; + "501-1000" = range 501 1000; + "501+" = range 501 null; + "1001-2500" = range 1001 2500; + "2501-5000" = range 2501 5000; + "5001+" = range 5001 null; + } ) rebuildCountByKernel ); }