ci(pr-coments): handle partial successes in pr-comments workflow #15294

slonka · 2025-12-16T08:42:24Z

Motivation

If there is a partial failure like this https://github.com/kumahq/kuma/actions/runs/20259928702 it still might be valid to push format changes.

Implementation information

Made it possible to submit partial success, added 👎 when failure.

Signed-off-by: slonka <[email protected]>

.github/workflows/pr-comments.yaml

github-actions · 2025-12-16T08:51:27Z

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
IPv6 is taken into account (.e.g: no string concatenation of host port)
Tests (Unit test, E2E tests, manual test on universal and k8s)
- Don't forget ci/ labels to run additional/fewer tests
Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

Copilot

Pull request overview

This PR enhances the pr-comments workflow to better handle partial failures when formatting code or updating golden files via PR comments. Previously, if any step failed, no changes would be pushed. Now, the workflow can push successful changes even if some steps fail, and provides better feedback through GitHub reactions (👍 for success, 👎 for failure).

Key changes:

Split formatting and golden file update steps into separate execution and commit phases with continue-on-error: true
Added conditional logic to commit and push changes only when the corresponding step doesn't skip
Implemented dual reaction system: success reaction when no failures occur, failure reaction when any step fails

.github/workflows/pr-comments.yaml

Copilot · 2025-12-16T09:47:30Z

.github/workflows/pr-comments.yaml

+            echo "FORMAT_PUSHED=true" >> "$GITHUB_ENV"
+          fi

      # Update all golden files except transparent proxy tests if /golden_files is in the comment
      - name: "Update golden files (excluding transparent proxy)"
        if: contains(github.event.comment.body, '/golden_files')
+        id: golden-files
+        continue-on-error: true
        run: make test UPDATE_GOLDEN_FILES=true

+      - name: "Commit and push golden files changes"
+        if: contains(github.event.comment.body, '/golden_files') && steps.golden-files.outcome != 'skipped'
+        continue-on-error: true
+        env:
+          GITHUB_TOKEN: ${{ steps.github-app-token.outputs.token }}
+        run: |
+          if git diff --exit-code --stat; then
+            echo "No golden files changes detected"
+          else
+            git config user.name "${GH_USER}"
+            git config user.email "${GH_EMAIL}"
+            git commit -s -m "fix(ci): update golden files" .
+            git push
+            echo "GOLDEN_FILES_PUSHED=true" >> "$GITHUB_ENV"
+          fi
+
      # Update only transparent proxy golden files if /golden_files_tproxy is in the comment
      - name: "Update transparent proxy golden files"
        if: contains(github.event.comment.body, '/golden_files_tproxy')
+        id: golden-files-tproxy
+        continue-on-error: true
        run: make test/transparentproxy UPDATE_GOLDEN_FILES=true

-      - name: commit and push fixes
+      - name: "Commit and push transparent proxy golden files changes"
+        if: contains(github.event.comment.body, '/golden_files_tproxy') && steps.golden-files-tproxy.outcome != 'skipped'
+        continue-on-error: true
        env:
          GITHUB_TOKEN: ${{ steps.github-app-token.outputs.token }}
        run: |
          if git diff --exit-code --stat; then
-            echo "No change detected, skipping git push"
+            echo "No transparent proxy golden files changes detected"
          else
            git config user.name "${GH_USER}"
            git config user.email "${GH_EMAIL}"
-            git commit -s -m "fix(ci): format files" .
+            git commit -s -m "fix(ci): update transparent proxy golden files" .
            git push
+            echo "TPROXY_GOLDEN_FILES_PUSHED=true" >> "$GITHUB_ENV"


The environment variables FORMAT_PUSHED, GOLDEN_FILES_PUSHED, and TPROXY_GOLDEN_FILES_PUSHED are set but never used. If these were intended for tracking or debugging purposes, they would not be visible in subsequent workflow runs or steps since environment variables set with GITHUB_ENV only persist within the current job.

If these variables are not needed, consider removing them to reduce complexity. If they are needed for future use, add a comment explaining their purpose.

.github/workflows/pr-comments.yaml

Signed-off-by: slonka <[email protected]>

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

Copilot · 2025-12-16T10:26:01Z

.github/workflows/pr-comments.yaml

+          if [ "${{ steps.format.outcome }}" != "" ]; then
+            if [ "${{ steps.format.outcome }}" == "success" ]; then
+              SUMMARY="${SUMMARY}✅ **Format**: Succeeded and pushed\n"
+            elif [ "${{ steps.format.outcome }}" == "failure" ]; then
+              SUMMARY="${SUMMARY}❌ **Format**: Failed\n"
+            fi
+          fi
+
+          if [ "${{ steps.golden-files.outcome }}" != "" ]; then
+            if [ "${{ steps.golden-files.outcome }}" == "success" ]; then
+              SUMMARY="${SUMMARY}✅ **Golden files**: Succeeded and pushed\n"
+            elif [ "${{ steps.golden-files.outcome }}" == "failure" ]; then
+              SUMMARY="${SUMMARY}❌ **Golden files**: Failed\n"
+            fi
+          fi
+
+          if [ "${{ steps.golden-files-tproxy.outcome }}" != "" ]; then
+            if [ "${{ steps.golden-files-tproxy.outcome }}" == "success" ]; then
+              SUMMARY="${SUMMARY}✅ **Transparent proxy golden files**: Succeeded and pushed\n"
+            elif [ "${{ steps.golden-files-tproxy.outcome }}" == "failure" ]; then
+              SUMMARY="${SUMMARY}❌ **Transparent proxy golden files**: Failed\n"
+            fi
+          fi


The condition checking for empty outcome (!= "") doesn't distinguish between a step that was skipped versus one that ran. When a step is skipped (because its if condition is false), the outcome will be an empty string, but this check will treat it the same as if it never existed. This means if someone comments /format only, the summary will incorrectly show results for golden files steps that were never triggered.

Consider checking if the step was actually triggered by verifying the corresponding comment body contains the command, or use the conclusion field which differentiates between skipped and not-run steps.

Copilot · 2025-12-16T10:26:01Z

.github/workflows/pr-comments.yaml

+            if [ "${{ steps.format.outcome }}" == "success" ]; then
+              SUMMARY="${SUMMARY}✅ **Format**: Succeeded and pushed\n"
+            elif [ "${{ steps.format.outcome }}" == "failure" ]; then
+              SUMMARY="${SUMMARY}❌ **Format**: Failed\n"
+            fi
+          fi
+
+          if [ "${{ steps.golden-files.outcome }}" != "" ]; then
+            if [ "${{ steps.golden-files.outcome }}" == "success" ]; then
+              SUMMARY="${SUMMARY}✅ **Golden files**: Succeeded and pushed\n"
+            elif [ "${{ steps.golden-files.outcome }}" == "failure" ]; then
+              SUMMARY="${SUMMARY}❌ **Golden files**: Failed\n"
+            fi
+          fi
+
+          if [ "${{ steps.golden-files-tproxy.outcome }}" != "" ]; then
+            if [ "${{ steps.golden-files-tproxy.outcome }}" == "success" ]; then
+              SUMMARY="${SUMMARY}✅ **Transparent proxy golden files**: Succeeded and pushed\n"
+            elif [ "${{ steps.golden-files-tproxy.outcome }}" == "failure" ]; then
+              SUMMARY="${SUMMARY}❌ **Transparent proxy golden files**: Failed\n"


The summary message incorrectly states "Succeeded and pushed" for all successful outcomes, but the commit-and-push steps have continue-on-error: true, meaning they could fail silently after the main step succeeds. If the git push fails (e.g., due to permissions or conflicts), the summary will still show "Succeeded and pushed" even though nothing was actually pushed.

Consider checking the outcome of the commit-and-push steps as well, or remove continue-on-error from those steps so failures are properly reported. Alternatively, update the message to say "Succeeded" without implying the push completed successfully.

Copilot · 2025-12-16T10:26:02Z

.github/workflows/pr-comments.yaml

+          steps.format.outcome != 'failure' &&
+          steps.golden-files.outcome != 'failure' &&
+          steps.golden-files-tproxy.outcome != 'failure'


The condition uses string inequality checks that could be simplified and made more explicit. Currently, checking steps.format.outcome != 'failure' for each step means that skipped steps (with empty outcomes) are treated as successes.

Consider using explicit positive checks like (steps.format.outcome == 'success' || steps.golden-files.outcome == 'success' || steps.golden-files-tproxy.outcome == 'success') to only add the success reaction when at least one operation actually succeeded.

Suggested change

steps.format.outcome != 'failure' &&

steps.golden-files.outcome != 'failure' &&

steps.golden-files-tproxy.outcome != 'failure'

(

steps.format.outcome == 'success' ||

steps.golden-files.outcome == 'success' ||

steps.golden-files-tproxy.outcome == 'success'

)

Signed-off-by: slonka <[email protected]>

fix(ci): handle partial successes in pr-comments workflow

c4a0c5d

Signed-off-by: slonka <[email protected]>

slonka requested a review from a team as a code owner December 16, 2025 08:42

slonka added the ci/skip-test PR: Don't run unit and e2e tests (maybe this is just a doc change) label Dec 16, 2025

slonka requested review from Automaat and lobkovilya December 16, 2025 08:42

Automaat reviewed Dec 16, 2025

View reviewed changes

.github/workflows/pr-comments.yaml Outdated Show resolved Hide resolved

Automaat reviewed Dec 16, 2025

View reviewed changes

.github/workflows/pr-comments.yaml Show resolved Hide resolved

Automaat requested a review from Copilot December 16, 2025 09:41

Copilot started reviewing on behalf of Automaat December 16, 2025 09:44 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

slonka changed the title ~~fix(ci): handle partial successes in pr-comments workflow~~ ci(pr-coments): handle partial successes in pr-comments workflow Dec 16, 2025

ci(pr-comments): address comments

f72c032

Signed-off-by: slonka <[email protected]>

slonka requested a review from Copilot December 16, 2025 10:12

Copilot started reviewing on behalf of slonka December 16, 2025 10:13 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

ci(pr-comments): address comments 2

e85e819

Signed-off-by: slonka <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ci(pr-coments): handle partial successes in pr-comments workflow #15294

ci(pr-coments): handle partial successes in pr-comments workflow #15294

slonka commented Dec 16, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 16, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Dec 16, 2025

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 16, 2025

Uh oh!

Copilot AI Dec 16, 2025

Uh oh!

Copilot AI Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ci(pr-coments): handle partial successes in pr-comments workflow #15294

Are you sure you want to change the base?

ci(pr-coments): handle partial successes in pr-comments workflow #15294

Conversation

slonka commented Dec 16, 2025

Motivation

Implementation information

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 16, 2025

Reviewer Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants