Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 12 additions & 8 deletions .github/scripts/tests/post-pr-review.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -284,14 +284,18 @@ for wf in "${WORKFLOWS[@]}"; do
check "$name: model step is continue-on-error" grep -q '^ continue-on-error: true$' "$wf"
check "$name: a run that fails before publishing explains itself in its section" \
grep -q -- '--only-if-unstamped' "$wf"
# Any outcome gate is wrong here: the action exits zero both when the job is
# cancelled by its own timeout and when the model reports a failed publish and
# stops. --only-if-unstamped makes the note a no-op once the section is
# stamped at this head, so it runs unconditionally.
check "$name: the note runs on every end of the job" \
grep -qF " if: always()" "$wf"
check "$name: and is not gated on the model step's outcome" \
bash -c "! grep -qE \"if: always\\(\\) && steps\\.[a-z]+\\.outcome\" '$wf'"
# The note has to survive a cancelled job and a zero-exit run that never
# published, and it has to stay silent when the action never ran at all --
# otherwise every pull request on a repository without the identifiers gets a
# review saying a check did not finish. Scoped to the note step, because the
# summary step below it is legitimately a bare always().
note="sed -n '/- name: Note the unfinished review/,/- name: Report outcome/p' '$wf'"
check "$name: the note survives a cancelled or failed step" \
bash -c "$note | grep -q \"outcome == 'cancelled'\" && $note | grep -q \"outcome == 'failure'\""
check "$name: and still fires when the action ran and published nothing" \
bash -c "$note | grep -qF \"outputs.conclusion != ''\""
check "$name: and is silent when the action never ran" \
bash -c "! $note | grep -qE '^ if: always\\(\\)\$'"
check "$name: the step that writes it cannot turn the PR red either" \
test "$(grep -c '^ continue-on-error: true$' "$wf")" = 2
# shellcheck disable=SC2016 # matching the literal shell in the workflow.
Expand Down
24 changes: 16 additions & 8 deletions .github/workflows/pr-review-suggestions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,21 @@ jobs:
# This step is continue-on-error for the same reason the model step is:
# neither can be allowed to turn a PR red.
- name: Note the unfinished review
# always(), with no outcome check: the job's own timeout cancels
# the job, and the action also exits zero when the model reports a
# failed publish and stops. Both leave the section reading as if
# this check had never started. --only-if-unstamped makes this a
# no-op whenever the section is already stamped at this head, so
# running it unconditionally is safe.
if: always()
# Three ways a run ends without publishing, and only two of them
# are an unfinished review. The action sets its conclusion output
# when it ran, so a set conclusion covers the case where the model
# reported a failed publish and stopped, which exits zero. A
# cancelled or failed step covers the job hitting its own timeout.
# An empty conclusion with the step still green means the action
# never ran at all -- the identifiers are unset on this repository,
# or the pull request changes this file -- and saying a review did
# not finish would be wrong and would land on every pull request.
# --only-if-unstamped makes this a no-op once the section is
# stamped at this head.
if: >-
always() && (steps.suggest.outputs.conclusion != ''
|| steps.suggest.outcome == 'failure'
|| steps.suggest.outcome == 'cancelled')
continue-on-error: true
env:
GH_TOKEN: ${{ github.token }}
Expand All @@ -165,7 +173,7 @@ jobs:
if [ "$OUTCOME" = "failure" ]; then
echo "**The review started but did not finish**, so it posted nothing about the diff. Hitting \`--max-turns\` looks like this, and so does an API failure. The \`Suggest\` step log has the cause."
elif [ -z "$CONCLUSION" ]; then
echo "**The review did not run.** Expected on a PR that changes this workflow file — the action requires the workflow to match the copy on the default branch. Any other cause is in the \`Suggest\` step log."
echo "**The review did not run.** Expected when `PR_REVIEW_FEDERATION_RULE_ID`, `PR_REVIEW_ORGANIZATION_ID`, `PR_REVIEW_SERVICE_ACCOUNT_ID` and `PR_REVIEW_WORKSPACE_ID` are not set as variables on this repository, and on a PR that changes this workflow file — the action requires the workflow to match the copy on the default branch. Any other cause is in the \`Suggest\` step log."
elif [ "$CONCLUSION" = "success" ]; then
echo "Review ran."
else
Expand Down
24 changes: 16 additions & 8 deletions .github/workflows/pr-test-coverage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,21 @@ jobs:
# This step is continue-on-error for the same reason the model step is:
# neither can be allowed to turn a PR red.
- name: Note the unfinished review
# always(), with no outcome check: the job's own timeout cancels
# the job, and the action also exits zero when the model reports a
# failed publish and stops. Both leave the section reading as if
# this check had never started. --only-if-unstamped makes this a
# no-op whenever the section is already stamped at this head, so
# running it unconditionally is safe.
if: always()
# Three ways a run ends without publishing, and only two of them
# are an unfinished review. The action sets its conclusion output
# when it ran, so a set conclusion covers the case where the model
# reported a failed publish and stopped, which exits zero. A
# cancelled or failed step covers the job hitting its own timeout.
# An empty conclusion with the step still green means the action
# never ran at all -- the identifiers are unset on this repository,
# or the pull request changes this file -- and saying a review did
# not finish would be wrong and would land on every pull request.
# --only-if-unstamped makes this a no-op once the section is
# stamped at this head.
if: >-
always() && (steps.review.outputs.conclusion != ''
|| steps.review.outcome == 'failure'
|| steps.review.outcome == 'cancelled')
continue-on-error: true
env:
GH_TOKEN: ${{ github.token }}
Expand All @@ -175,7 +183,7 @@ jobs:
if [ "$OUTCOME" = "failure" ]; then
echo "**The review started but did not finish**, so it posted nothing about the diff. Hitting \`--max-turns\` looks like this, and so does an API failure. The \`Review\` step log has the cause."
elif [ -z "$CONCLUSION" ]; then
echo "**The review did not run.** Expected on a PR that changes this workflow file, since the action requires the workflow to match the copy on the default branch. Any other cause is in the \`Review\` step log."
echo "**The review did not run.** Expected when `PR_REVIEW_FEDERATION_RULE_ID`, `PR_REVIEW_ORGANIZATION_ID`, `PR_REVIEW_SERVICE_ACCOUNT_ID` and `PR_REVIEW_WORKSPACE_ID` are not set as variables on this repository, and on a PR that changes this workflow file, since the action requires the workflow to match the copy on the default branch. Any other cause is in the \`Review\` step log."
elif [ "$CONCLUSION" = "success" ]; then
echo "Review ran."
else
Expand Down
Loading