From 3f4087fd5de10dfb98c01c9a502903a544843c06 Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Fri, 18 Apr 2025 16:00:03 -0700 Subject: [PATCH 01/15] Surface curl errors Errors in the invocation of `curl` were not being reported. Added `-f` flag to `curl`, return status checking, and printing the response body in case of errors. --- dev_tools/ci/size-labeler.sh | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/dev_tools/ci/size-labeler.sh b/dev_tools/ci/size-labeler.sh index 387d9c80501..25db8d31eb5 100755 --- a/dev_tools/ci/size-labeler.sh +++ b/dev_tools/ci/size-labeler.sh @@ -81,14 +81,26 @@ function jq_file() { function api_call() { local -r endpoint="${1// /%20}" # love that our label names have spaces... local -r uri="https://api.github.com/repos/${GITHUB_REPOSITORY}" - info "Calling: ${uri}/${endpoint}" - curl -sSL \ + local -r url="${uri}/${endpoint}" + local -r curl_opts=( + -fsSL + --connect-timeout 10 --max-time 20 -H "Authorization: token ${GITHUB_TOKEN}" \ -H "Accept: application/vnd.github.v3.json" \ -H "X-GitHub-Api-Version:2022-11-28" \ -H "Content-Type: application/json" \ - "${@:2}" \ - "${uri}/${endpoint}" + ) + info "Calling: ${url}" + set +e + response_body=$(curl "${curl_opts[@]}" "${@:2}" "${url}") + local exit_status=$? + set -e + if [[ $exit_status -ne 0 ]]; then + error "GitHub API call failed (curl exit $exit_status) for ${url}" + cat "$response_body" + exit $exit_status + fi + return "$response_body" } function compute_changes() { From 00bcb659078219d7ea2914f3e0124dd46a2cf318 Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Fri, 18 Apr 2025 16:20:28 -0700 Subject: [PATCH 02/15] Use echo, not return --- dev_tools/ci/size-labeler.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev_tools/ci/size-labeler.sh b/dev_tools/ci/size-labeler.sh index 25db8d31eb5..7d03d7ea1e7 100755 --- a/dev_tools/ci/size-labeler.sh +++ b/dev_tools/ci/size-labeler.sh @@ -100,7 +100,7 @@ function api_call() { cat "$response_body" exit $exit_status fi - return "$response_body" + echo "$response_body" } function compute_changes() { From 095cf41989b0611a5dc473d85910449a250a67b8 Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Fri, 18 Apr 2025 16:30:45 -0700 Subject: [PATCH 03/15] Adjust style of variable references to match existing code --- dev_tools/ci/size-labeler.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dev_tools/ci/size-labeler.sh b/dev_tools/ci/size-labeler.sh index 7d03d7ea1e7..bb9b3b17bdb 100755 --- a/dev_tools/ci/size-labeler.sh +++ b/dev_tools/ci/size-labeler.sh @@ -79,6 +79,7 @@ function jq_file() { } function api_call() { + set -x local -r endpoint="${1// /%20}" # love that our label names have spaces... local -r uri="https://api.github.com/repos/${GITHUB_REPOSITORY}" local -r url="${uri}/${endpoint}" @@ -97,10 +98,10 @@ function api_call() { set -e if [[ $exit_status -ne 0 ]]; then error "GitHub API call failed (curl exit $exit_status) for ${url}" - cat "$response_body" + cat "${response_body}" exit $exit_status fi - echo "$response_body" + echo "${response_body}" } function compute_changes() { From 5a7167114c3051a8ee493eff378231d52b9e8f1a Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Mon, 21 Apr 2025 07:58:42 -0700 Subject: [PATCH 04/15] Fix a couple of mistakes and improve DRYness - `cat` should have been `echo` - remove debugging setting - refactor a few lines in api_call() --- dev_tools/ci/size-labeler.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dev_tools/ci/size-labeler.sh b/dev_tools/ci/size-labeler.sh index bb9b3b17bdb..dddfcbeeafe 100755 --- a/dev_tools/ci/size-labeler.sh +++ b/dev_tools/ci/size-labeler.sh @@ -79,7 +79,6 @@ function jq_file() { } function api_call() { - set -x local -r endpoint="${1// /%20}" # love that our label names have spaces... local -r uri="https://api.github.com/repos/${GITHUB_REPOSITORY}" local -r url="${uri}/${endpoint}" @@ -96,12 +95,11 @@ function api_call() { response_body=$(curl "${curl_opts[@]}" "${@:2}" "${url}") local exit_status=$? set -e + echo "${response_body}" if [[ $exit_status -ne 0 ]]; then error "GitHub API call failed (curl exit $exit_status) for ${url}" - cat "${response_body}" exit $exit_status fi - echo "${response_body}" } function compute_changes() { From 96b050c03283be6a45f6c44c29f1a2ef466b9565 Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Tue, 22 Apr 2025 14:17:24 -0700 Subject: [PATCH 05/15] Get rid of token parameter for workflow_dispatch I never seem to need it. --- .github/workflows/pr-labeler.yaml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/pr-labeler.yaml b/.github/workflows/pr-labeler.yaml index 1f0e03b4a11..8f2f6317020 100644 --- a/.github/workflows/pr-labeler.yaml +++ b/.github/workflows/pr-labeler.yaml @@ -37,9 +37,6 @@ on: description: 'The PR number of the PR to label:' type: string required: true - token: - description: 'Token to use for authentication:' - type: string # Declare default permissions as read only. permissions: read-all @@ -63,7 +60,7 @@ jobs: id: label continue-on-error: true env: - GITHUB_TOKEN: ${{inputs.token || github.token}} + GITHUB_TOKEN: ${{github.token}} run: ./dev_tools/ci/size-labeler.sh |& tee action.out - name: Detect and report errors in labeler action From 36ec815f325559035ba653dbf9d0a3435f18f602 Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Tue, 22 Apr 2025 14:18:08 -0700 Subject: [PATCH 06/15] Add option to set Bash tracing This approach is a simple way to debug all shell scripts and commands. --- .github/workflows/pr-labeler.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/pr-labeler.yaml b/.github/workflows/pr-labeler.yaml index 8f2f6317020..f364b6c8dc9 100644 --- a/.github/workflows/pr-labeler.yaml +++ b/.github/workflows/pr-labeler.yaml @@ -37,6 +37,9 @@ on: description: 'The PR number of the PR to label:' type: string required: true + trace: + description: 'Turn on shell script debugging' + type: boolean # Declare default permissions as read only. permissions: read-all @@ -52,6 +55,7 @@ jobs: pull-requests: write env: PR_NUMBER: ${{inputs.pr-number || github.event.pull_request.number}} + SHELLOPTS: ${{inputs.trace && 'xtrace' || '' }} steps: - name: Check out a copy of the git repository uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 From 16b6020f7720aa2867c3472e7ff44358584c8b1e Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Tue, 22 Apr 2025 20:38:28 -0700 Subject: [PATCH 07/15] Speed up git checkouts by making them sparse --- .github/workflows/pr-labeler.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/pr-labeler.yaml b/.github/workflows/pr-labeler.yaml index f364b6c8dc9..79c6dc1b5b1 100644 --- a/.github/workflows/pr-labeler.yaml +++ b/.github/workflows/pr-labeler.yaml @@ -59,6 +59,9 @@ jobs: steps: - name: Check out a copy of the git repository uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + sparse-checkout: | + ./dev_tools/ci/size-labeler.sh - name: Label the PR with a size label id: label From 15424d159762ee528a62387667cb40e5347721bf Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 24 Apr 2025 17:26:03 -0700 Subject: [PATCH 08/15] Clean up temporary files created in jq_stdin And make sure it still returns the jq status code. --- dev_tools/ci/size-labeler.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dev_tools/ci/size-labeler.sh b/dev_tools/ci/size-labeler.sh index dddfcbeeafe..e4ab99d3e82 100755 --- a/dev_tools/ci/size-labeler.sh +++ b/dev_tools/ci/size-labeler.sh @@ -59,9 +59,12 @@ function jq_stdin() { local infile infile="$(mktemp)" readonly infile + local jq_status=0 cat >"${infile}" - jq_file "$@" "${infile}" + jq_file "$@" "${infile}" || jq_status="${?}" + rm "${infile}" + return "${jq_status}" } function jq_file() { From 544afab7417ea2472d6fb8524aca2a9a967401f6 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 24 Apr 2025 17:30:29 -0700 Subject: [PATCH 09/15] Rewrite api_call to output response from the server when call fails Write error response to stderr as stdout may be captured by the caller. Use `cat` for outputting response as `echo` may translate escape sequences. Make sure api_call returns the curl status code. --- dev_tools/ci/size-labeler.sh | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/dev_tools/ci/size-labeler.sh b/dev_tools/ci/size-labeler.sh index e4ab99d3e82..2f7b150ca7c 100755 --- a/dev_tools/ci/size-labeler.sh +++ b/dev_tools/ci/size-labeler.sh @@ -84,25 +84,28 @@ function jq_file() { function api_call() { local -r endpoint="${1// /%20}" # love that our label names have spaces... local -r uri="https://api.github.com/repos/${GITHUB_REPOSITORY}" - local -r url="${uri}/${endpoint}" - local -r curl_opts=( - -fsSL - --connect-timeout 10 --max-time 20 + local response + local curl_status=0 + info "Calling: ${uri}/${endpoint}" + response="$(curl -sSL \ + --fail-with-body \ + --connect-timeout 10 --max-time 20 \ -H "Authorization: token ${GITHUB_TOKEN}" \ -H "Accept: application/vnd.github.v3.json" \ -H "X-GitHub-Api-Version:2022-11-28" \ -H "Content-Type: application/json" \ - ) - info "Calling: ${url}" - set +e - response_body=$(curl "${curl_opts[@]}" "${@:2}" "${url}") - local exit_status=$? - set -e - echo "${response_body}" - if [[ $exit_status -ne 0 ]]; then - error "GitHub API call failed (curl exit $exit_status) for ${url}" - exit $exit_status + "${@:2}" \ + "${uri}/${endpoint}" + )" || curl_status="${?}" + if [[ -n "${response}" ]]; then + cat <<<"${response}" fi + if (( curl_status )); then + error "GitHub API call failed (curl exit $curl_status) for ${uri}/${endpoint}" + error "Response body:" + cat >&2 <<<"${response}" + fi + return "${curl_status}" } function compute_changes() { From 8c90b022ef1fca7a7ff0f742ba01be758b95377e Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 24 Apr 2025 17:36:21 -0700 Subject: [PATCH 10/15] Terminate early if api_call returns error status Avoid using `cmd <<<"$(api_call args)"` expressions which ignore `api_call` failure. --- dev_tools/ci/size-labeler.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dev_tools/ci/size-labeler.sh b/dev_tools/ci/size-labeler.sh index 2f7b150ca7c..a5a9e2591ba 100755 --- a/dev_tools/ci/size-labeler.sh +++ b/dev_tools/ci/size-labeler.sh @@ -111,9 +111,11 @@ function api_call() { function compute_changes() { local -r pr="$1" + local response local change_info local -r keys_filter='with_entries(select([.key] | inside(["changes", "filename"])))' - change_info="$(jq_stdin "map(${keys_filter})" <<<"$(api_call "pulls/${pr}/files")")" + response="$(api_call "pulls/${pr}/files")" + change_info="$(jq_stdin "map(${keys_filter})" <<<"${response}")" local files total_changes readarray -t files < <(jq_stdin -c '.[]' <<<"${change_info}") @@ -148,8 +150,10 @@ function get_size_label() { function prune_stale_labels() { local -r pr="$1" local -r size_label="$2" + local response local existing_labels - existing_labels="$(jq_stdin -r '.labels[] | .name' <<<"$(api_call "pulls/${pr}")")" + response="$(api_call "pulls/${pr}")" + existing_labels="$(jq_stdin -r '.labels[] | .name' <<<"${response}")" readarray -t existing_labels <<<"${existing_labels}" local correctly_labeled=false From d6de407c256084fa7e535d1824c00d379924ec24 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 24 Apr 2025 17:42:54 -0700 Subject: [PATCH 11/15] Do not discard stderr from api_call This is where it sends diagnostics if any. --- dev_tools/ci/size-labeler.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev_tools/ci/size-labeler.sh b/dev_tools/ci/size-labeler.sh index a5a9e2591ba..defcde544dc 100755 --- a/dev_tools/ci/size-labeler.sh +++ b/dev_tools/ci/size-labeler.sh @@ -168,7 +168,7 @@ function prune_stale_labels() { # If there is another size label, we need to remove it if [[ -v "LIMITS[${label}]" ]]; then info "Label '${label}' is stale, removing it." - api_call "issues/${pr}/labels/${label}" -X DELETE &>/dev/null + api_call "issues/${pr}/labels/${label}" -X DELETE >/dev/null continue fi info "Label '${label}' is unknown, leaving it." @@ -215,7 +215,7 @@ function main() { correctly_labeled="$(prune_stale_labels "$PR_NUMBER" "${size_label}")" if [[ "${correctly_labeled}" != true ]]; then - api_call "issues/$PR_NUMBER/labels" -X POST -d "{\"labels\":[\"${size_label}\"]}" &>/dev/null + api_call "issues/$PR_NUMBER/labels" -X POST -d "{\"labels\":[\"${size_label}\"]}" >/dev/null info "Added label '${size_label}'" fi } From 37bf2f660b63c44ee12aeb6ba41d8deb78b80098 Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Mon, 28 Apr 2025 18:12:35 -0700 Subject: [PATCH 12/15] Switch to a PAT instead of GITHUB_TOKEN Once again, got bitten by the fact that adding labels to an issue doesn't work with `GITHUB_TOKEN` C.f. https://docs.github.com/en/rest/issues/labels?apiVersion=2022-11-28#add-labels-to-an-issue --- .github/workflows/pr-labeler.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr-labeler.yaml b/.github/workflows/pr-labeler.yaml index 79c6dc1b5b1..c35bc996a2a 100644 --- a/.github/workflows/pr-labeler.yaml +++ b/.github/workflows/pr-labeler.yaml @@ -67,7 +67,7 @@ jobs: id: label continue-on-error: true env: - GITHUB_TOKEN: ${{github.token}} + GITHUB_TOKEN: ${{secrets.PR_LABELER_TOKEN}} run: ./dev_tools/ci/size-labeler.sh |& tee action.out - name: Detect and report errors in labeler action From 5720edd04f2ba72a1f3ed189a093dee48cd5f426 Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Mon, 28 Apr 2025 18:13:50 -0700 Subject: [PATCH 13/15] Add comments to explain some non-obvious parts This is for future readers. --- .github/workflows/pr-labeler.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/pr-labeler.yaml b/.github/workflows/pr-labeler.yaml index c35bc996a2a..e1906cd0de5 100644 --- a/.github/workflows/pr-labeler.yaml +++ b/.github/workflows/pr-labeler.yaml @@ -55,6 +55,8 @@ jobs: pull-requests: write env: PR_NUMBER: ${{inputs.pr-number || github.event.pull_request.number}} + # The next var is used by Bash. We add 'xtrace' to the options if this run + # is a workflow_dispatch invocation and the user set the 'trace' option. SHELLOPTS: ${{inputs.trace && 'xtrace' || '' }} steps: - name: Check out a copy of the git repository @@ -68,6 +70,7 @@ jobs: continue-on-error: true env: GITHUB_TOKEN: ${{secrets.PR_LABELER_TOKEN}} + # N.B.: GitHub runners set the `pipefail` option by default. run: ./dev_tools/ci/size-labeler.sh |& tee action.out - name: Detect and report errors in labeler action From fc7d3dca3b483a25f302533f3068a5187cedb8be Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Mon, 28 Apr 2025 19:53:30 -0700 Subject: [PATCH 14/15] Take out pipe Per [review comments by @pavol](https://github.com/quantumlib/Cirq/pull/7288#discussion_r2065212396), it is not doing what I thought it was doing. Plus, we don't really need the final output step. --- .github/workflows/pr-labeler.yaml | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/.github/workflows/pr-labeler.yaml b/.github/workflows/pr-labeler.yaml index e1906cd0de5..c6110a8dfca 100644 --- a/.github/workflows/pr-labeler.yaml +++ b/.github/workflows/pr-labeler.yaml @@ -70,15 +70,4 @@ jobs: continue-on-error: true env: GITHUB_TOKEN: ${{secrets.PR_LABELER_TOKEN}} - # N.B.: GitHub runners set the `pipefail` option by default. - run: ./dev_tools/ci/size-labeler.sh |& tee action.out - - - name: Detect and report errors in labeler action - if: steps.label.outcome == 'failure' || steps.label.outcome == 'error' - run: | - { - echo "

‼️ Failed to label PR ${PR_NUMBER}

" - echo "
"
-          cat action.out
-          echo "
" - } >> "$GITHUB_STEP_SUMMARY" + run: ./dev_tools/ci/size-labeler.sh From 0b6c8a5f4139aad4cc3ba6abde44e082c29f2ba8 Mon Sep 17 00:00:00 2001 From: Michael Hucka Date: Mon, 28 Apr 2025 20:07:29 -0700 Subject: [PATCH 15/15] Go back to using pull_request_target & GITHUB_TOKEN --- .github/workflows/pr-labeler.yaml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pr-labeler.yaml b/.github/workflows/pr-labeler.yaml index c6110a8dfca..025831731f3 100644 --- a/.github/workflows/pr-labeler.yaml +++ b/.github/workflows/pr-labeler.yaml @@ -25,7 +25,10 @@ run-name: >- Label pull request ${{github.event.pull_request.number}} by ${{github.actor}} on: - pull_request: + # Note: do not copy-paste this workflow with `pull_request_target` left as-is. + # Its use here is a special case where security implications are understood. + # Workflows should normally use `pull_request` instead. + pull_request_target: types: - opened - synchronize @@ -69,5 +72,5 @@ jobs: id: label continue-on-error: true env: - GITHUB_TOKEN: ${{secrets.PR_LABELER_TOKEN}} + GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}} run: ./dev_tools/ci/size-labeler.sh