Skip to content

Improve reporting of errors & efficiency of pr-size-labeler #7288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Apr 29, 2025
Merged

Improve reporting of errors & efficiency of pr-size-labeler #7288

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3f4087f
Surface curl errors
mhucka Apr 18, 2025
00bcb65
Use echo, not return
mhucka Apr 18, 2025
095cf41
Adjust style of variable references to match existing code
mhucka Apr 18, 2025
5a71671
Fix a couple of mistakes and improve DRYness
mhucka Apr 21, 2025
96b050c
Get rid of token parameter for workflow_dispatch
mhucka Apr 22, 2025
36ec815
Add option to set Bash tracing
mhucka Apr 22, 2025
16b6020
Speed up git checkouts by making them sparse
mhucka Apr 23, 2025
15424d1
Clean up temporary files created in jq_stdin
pavoljuhas Apr 25, 2025
544afab
Rewrite api_call to output response from the server when call fails
pavoljuhas Apr 25, 2025
8c90b02
Terminate early if api_call returns error status
pavoljuhas Apr 25, 2025
d6de407
Do not discard stderr from api_call
pavoljuhas Apr 25, 2025
37bf2f6
Switch to a PAT instead of GITHUB_TOKEN
mhucka Apr 29, 2025
5720edd
Add comments to explain some non-obvious parts
mhucka Apr 29, 2025
747fd6a
Merge branch 'main' into mh-fix-size-labeler
mhucka Apr 29, 2025
fc7d3dc
Take out pipe
mhucka Apr 29, 2025
0b6c8a5
Go back to using pull_request_target & GITHUB_TOKEN
mhucka Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions .github/workflows/pr-labeler.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ on:
description: 'The PR number of the PR to label:'
type: string
required: true
token:
description: 'Token to use for authentication:'
type: string
trace:
description: 'Turn on shell script debugging'
type: boolean

# Declare default permissions as read only.
permissions: read-all
Expand All @@ -55,15 +55,19 @@ jobs:
pull-requests: write
env:
PR_NUMBER: ${{inputs.pr-number || github.event.pull_request.number}}
SHELLOPTS: ${{inputs.trace && 'xtrace' || '' }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this used anywhere? If not please delete.

Copy link
Contributor Author

@mhucka mhucka Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. The environment variable is recognized by Bash: https://www.gnu.org/software/bash/manual/html_node/Bash-Variables.html#index-SHELLOPTS

Here what's happening is that when the Boolean trace is set by using workflow_dispatch, it causes SHELLOPTS to get the value xtrace. That's the same as using set -x: https://www.gnu.org/software/bash/manual/bash.html#index-set. Since the environment variable is set at the job level, all invocations get the SHELLOPTS value. The result is that all the Bash scripts and commands in the workflow get the equivalent of set -x, and thus output a trace of their steps. This is useful for debugging.

The beauty of this approach is that it doesn't require modifying any of the run: commands to add set -x, and it can be turned on and off at run time.

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
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
Copy link
Collaborator

@pavoljuhas pavoljuhas Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pipe here masks the exit status from size-labeler.sh.
Please either add set -o pipefail before or just take out the |& tee action.out and
change line 79 to something like "see the output above".

PS: Or consider deleting the Detect and report errors... step altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either add set -o pipefail before

I don't believe that's necessary; according to https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#defaultsrunshell the default Bash invocation used by GitHub runners is

bash --noprofile --norc -eo pipefail {0}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that's necessary; ...

I can't see any other reason why would labeler workflow report success at https://github.com/quantumlib/Cirq/actions/runs/14654500719/job/41127085113. The script terminated at the api_call line below and per job output it did not get to the next info "Added label..." line.

Cirq/dev_tools/ci/size-labeler.sh

Lines 217 to 220 in 747fd6a

if [[ "${correctly_labeled}" != true ]]; then
api_call "issues/$PR_NUMBER/labels" -X POST -d "{\"labels\":[\"${size_label}\"]}" >/dev/null
info "Added label '${size_label}'"
fi

On a second look, the shell command is actually listed as /usr/bin/bash -e {0}, which is one of the possibilities in your link. Regardless of that, the final step just repeats the size-labeler.sh output and is not really necessary. The output of this workflow is not important to contributors and is not worth the extra complexity and space for bugs like we get here.

Please consider taking it out and just have a simple execution of size-labeler.sh.

PS: if size-labeler remains flaky, we should intentionally let it report success, e.g.,

run: ./dev_tools/ci/size-labeler.sh || true

so that contributors are not confused by CI failure that has nothing to do with their work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/usr/bin/bash -e {0}

You're right! And now reading the GitHub docs, I think I understand what's happening: it seems that if there is no shell: property, it defaults to this, whereas shell: bash evidently makes it use the arguments that I thought it was using. (Why is it that way? I don't understand what the point of that is.)

Anyway, I'll change it as you propose.


- name: Detect and report errors in labeler action
Expand Down
33 changes: 27 additions & 6 deletions dev_tools/ci/size-labeler.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -81,22 +84,38 @@ 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 response
local curl_status=0
info "Calling: ${uri}/${endpoint}"
curl -sSL \
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" \
"${@: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() {
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}")
Expand Down Expand Up @@ -131,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
Expand All @@ -147,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."
Expand Down Expand Up @@ -194,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
}
Expand Down