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

mhucka · 2025-04-18T23:00:16Z

Errors in the invocation of curl were not being reported. Changes:

size-labeler.sh: Added -f flag to curl, added return status checking, and added printing the response body in case of errors.
pr-labeler.yaml: Removed token option in workflow_dispatch and added a trace option. The token turns out to be never (or rarely) needed, and the shell tracing is useful for debugging.

In addition, since the only file needed from the repository is the size-labeler.sh script, this also makes the git checkout in pr-labeler.yaml be sparse.

codecov · 2025-04-18T23:45:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.66%. Comparing base (4fa82fb) to head (0b6c8a5).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7288   +/-   ##
=======================================
  Coverage   98.66%   98.66%           
=======================================
  Files        1106     1106           
  Lines       96186    96186           
=======================================
  Hits        94906    94906           
  Misses       1280     1280

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

- `cat` should have been `echo` - remove debugging setting - refactor a few lines in api_call()

I never seem to need it.

This approach is a simple way to debug all shell scripts and commands.

And make sure it still returns the jq status code.

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.

Avoid using `cmd <<<"$(api_call args)"` expressions which ignore `api_call` failure.

This is where it sends diagnostics if any.

pavoljuhas

I think it is better if we use --fail-with-body so we get to see the error response from the server. There were quite a few places in the original script which masked failure status from api_call so I rewrote them in a way which should hopefully cause early exit.

Also, any error messages from api_call need to go to stderr, because stdout is captured by callers.

PTAL at my take on this, it can be reviewed commit-by-commit.

pavoljuhas · 2025-04-25T00:49:41Z

.github/workflows/pr-labeler.yaml

@@ -55,15 +55,19 @@ jobs:
      pull-requests: write
    env:
      PR_NUMBER: ${{inputs.pr-number || github.event.pull_request.number}}
+      SHELLOPTS: ${{inputs.trace && 'xtrace' || '' }}


is this used anywhere? If not please delete.

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.

pavoljuhas · 2025-04-25T00:59:05Z

Looks like we get some diagnostics - yay!
https://github.com/quantumlib/Cirq/actions/runs/14654500719/job/41127085113#step:3:18

pavoljuhas · 2025-04-25T01:14:57Z

.github/workflows/pr-labeler.yaml


      - 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


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.

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}

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.

/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.

mhucka · 2025-04-29T01:04:04Z

Looks like we get some diagnostics - yay! quantumlib/Cirq/actions/runs/14654500719/job/41127085113#step:3:18

Oh, argh. It looks like it needs a PAT after all.

I'll change the token it uses in the next commmit.

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

This is for future readers.

pavoljuhas

Please take out the last step that is obscuring exit status from labeler script. Otherwise LGTM.

pavoljuhas · 2025-04-29T02:20:57Z

Also, w/r to workflow permissions, we should change event type to pull_request_target and go back to stock GITHUB_TOKEN. Its docs in fact state

... This event allows your workflow to do things like label or comment on pull requests from forks. ...

Also the workflow will then run with the main branch sources which is certainly safer than hoping a PR does not introduce some mischievous actions such as exposing PAT.

@Pavol

Per [review comments by @Pavol](quantumlib#7288 (comment)), it is not doing what I thought it was doing. Plus, we don't really need the final output step.

mhucka · 2025-04-29T03:07:51Z

Also, w/r to workflow permissions, we should change event type to pull_request_target and go back to stock GITHUB_TOKEN. Its docs in fact state

... This event allows your workflow to do things like label or comment on pull requests from forks. ...

Also the workflow will then run with the main branch sources which is certainly safer than hoping a PR does not introduce some mischievous actions such as exposing PAT.

Using a pull_request_target was a bit controversial the last time, so I avoided it, but I agree this seems safe. New commit puts it back.

mhucka force-pushed the mh-fix-size-labeler branch 2 times, most recently from ee9985b to 7f4a83b Compare April 18, 2025 23:02

mhucka changed the title ~~mh fix size labeler~~ Improve reporting of API call errors in size-labeler.sh Apr 18, 2025

mhucka force-pushed the mh-fix-size-labeler branch from b465134 to c699fc9 Compare April 18, 2025 23:26

mhucka marked this pull request as ready for review April 18, 2025 23:31

mhucka requested review from vtomole and a team as code owners April 18, 2025 23:31

mhucka requested review from MichaelBroughton and pavoljuhas April 18, 2025 23:31

mhucka enabled auto-merge April 18, 2025 23:56

mhucka added 4 commits April 21, 2025 08:08

Surface curl errors

3f4087f

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.

Use echo, not return

00bcb65

Adjust style of variable references to match existing code

095cf41

Fix a couple of mistakes and improve DRYness

5a71671

- `cat` should have been `echo` - remove debugging setting - refactor a few lines in api_call()

mhucka force-pushed the mh-fix-size-labeler branch from 78e70dd to 5a71671 Compare April 21, 2025 15:09

mhucka added 3 commits April 22, 2025 14:17

Get rid of token parameter for workflow_dispatch

96b050c

I never seem to need it.

Add option to set Bash tracing

36ec815

This approach is a simple way to debug all shell scripts and commands.

Speed up git checkouts by making them sparse

16b6020

mhucka changed the title ~~Improve reporting of API call errors in size-labeler.sh~~ Improve reporting of errors & efficiency of pr-size-labeler Apr 23, 2025

pavoljuhas added 4 commits April 24, 2025 17:26

Clean up temporary files created in jq_stdin

15424d1

And make sure it still returns the jq status code.

Rewrite api_call to output response from the server when call fails

544afab

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.

Terminate early if api_call returns error status

8c90b02

Avoid using `cmd <<<"$(api_call args)"` expressions which ignore `api_call` failure.

Do not discard stderr from api_call

d6de407

This is where it sends diagnostics if any.

pavoljuhas disabled auto-merge April 25, 2025 00:46

pavoljuhas approved these changes Apr 25, 2025

View reviewed changes

pavoljuhas reviewed Apr 25, 2025

View reviewed changes

mhucka requested a review from pavoljuhas April 29, 2025 01:00

mhucka and others added 3 commits April 28, 2025 18:14

Switch to a PAT instead of GITHUB_TOKEN

37bf2f6

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

Add comments to explain some non-obvious parts

5720edd

This is for future readers.

Merge branch 'main' into mh-fix-size-labeler

747fd6a

pavoljuhas added size: S 10< lines changed <50 and removed size: S 10< lines changed <50 labels Apr 29, 2025

pavoljuhas requested changes Apr 29, 2025

View reviewed changes

mhucka added 2 commits April 28, 2025 20:07

Take out pipe

fc7d3dc

Per [review comments by @Pavol](quantumlib#7288 (comment)), it is not doing what I thought it was doing. Plus, we don't really need the final output step.

Go back to using pull_request_target & GITHUB_TOKEN

0b6c8a5

mhucka requested a review from pavoljuhas April 29, 2025 03:08

pavoljuhas approved these changes Apr 29, 2025

View reviewed changes

mhucka added this pull request to the merge queue Apr 29, 2025

Merged via the queue into quantumlib:main with commit f085651 Apr 29, 2025
69 of 73 checks passed

mhucka deleted the mh-fix-size-labeler branch April 29, 2025 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

mhucka commented Apr 18, 2025 •

edited

Loading

Uh oh!

codecov bot commented Apr 18, 2025 •

edited

Loading

Uh oh!

pavoljuhas left a comment

Uh oh!

pavoljuhas Apr 25, 2025

Uh oh!

mhucka Apr 29, 2025 •

edited

Loading

Uh oh!

pavoljuhas commented Apr 25, 2025

Uh oh!

pavoljuhas Apr 25, 2025 •

edited

Loading

Uh oh!

mhucka Apr 29, 2025

Uh oh!

pavoljuhas Apr 29, 2025

Uh oh!

mhucka Apr 29, 2025

Uh oh!

mhucka commented Apr 29, 2025

Uh oh!

pavoljuhas left a comment

Uh oh!

pavoljuhas commented Apr 29, 2025

Uh oh!

mhucka commented Apr 29, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

	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

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

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

Uh oh!

Conversation

mhucka commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pavoljuhas left a comment

Choose a reason for hiding this comment

Uh oh!

pavoljuhas Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

mhucka Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavoljuhas commented Apr 25, 2025

Uh oh!

pavoljuhas Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhucka Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

pavoljuhas Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

mhucka Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

mhucka commented Apr 29, 2025

Uh oh!

pavoljuhas left a comment

Choose a reason for hiding this comment

Uh oh!

pavoljuhas commented Apr 29, 2025

Uh oh!

mhucka commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhucka commented Apr 18, 2025 •

edited

Loading

codecov bot commented Apr 18, 2025 •

edited

Loading

mhucka Apr 29, 2025 •

edited

Loading

pavoljuhas Apr 25, 2025 •

edited

Loading

mhucka commented Apr 29, 2025 •

edited

Loading