Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR aims to always display the matrix output by moving the echo statement to the end of the workflow. However, the implementation contradicts this goal.
Key Changes:
- Removed matrix output display from immediately after generation (before error checking)
- Added matrix output display at the end of the "Print Info" step
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "" | ||
| echo "::group::===> Matrix Output" | ||
| cat matrix | ||
| echo "::endgroup::" |
There was a problem hiding this comment.
The PR title states "Always echo matrix output", but this change actually prevents the matrix output from being displayed when matrix generation fails.
When php-matrix composer returns a non-zero exit code, the script exits at line 121 before reaching this output block. The original placement (after line 117) would display the matrix output before exiting, which is useful for debugging failures.
Consider either:
- Keeping the matrix output before the error check (lines 119-121), or
- Removing the
exit 1on line 121 to allow the workflow to continue to the "Print Info" step
tangrufus
added
bug
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "" | ||
| echo "::group::===> Matrix Output" | ||
| stopMarker=$(uuidgen) | ||
| echo "::stop-commands::$stopMarker" | ||
| cat matrix | ||
| echo "::$stopMarker::" | ||
| echo "::endgroup::" |
There was a problem hiding this comment.
The matrix output will not be displayed when matrix generation fails. The "Print Info" step has if: ${{ !cancelled() }} (line 136), but when the "Generate Matrix" step exits with code 1 (line 121), the workflow will fail and skip subsequent steps by default.
To truly "always echo matrix output" as the PR title suggests, you need to either:
- Change the condition to
if: ${{ always() }}so it runs even after failures, or - Move the matrix output display back to the "Generate Matrix" step after line 117, before the error check
Option 2 would be closer to the original behavior while adding the stop-commands protection.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
action.yml
Outdated
| echo "" | ||
| echo "==> Matrix Output" | ||
| stopMarker=$(uuidgen) | ||
| echo "::stop-commands::$stopMarker" | ||
| cat matrix | ||
| echo "::$stopMarker::" |
There was a problem hiding this comment.
The matrix output now uses ::stop-commands:: instead of ::group::, but there's no comment explaining why this change is necessary. Consider adding a comment to clarify that ::stop-commands:: is used to prevent the matrix output (which may contain workflow command syntax like ::error:: or ::warning::) from being interpreted as actual GitHub Actions commands.
tangrufus
force-pushed
the
echo-output
branch
from
8f7729d to
8cd382d
Compare
tangrufus
force-pushed
the
echo-output
branch
from
0d34dc5 to
8cd382d
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.