Move PR comment creation to separate workflow on:pull_request_target #9
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the test reporting workflow by separating PR comment creation into a dedicated workflow that runs via workflow_run trigger. This improves security by using the correct permissions model for commenting on pull requests from forks.
- Moved PR comment creation from the main test workflow to a separate
pr-comment.ymlworkflow - Added artifact upload for PR number and CTRF test reports to enable cross-workflow communication
- Updated solution file to include all workflow files for better visibility
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/workflows/pr-comment.yml |
New workflow that triggers on test completion to post PR comments with appropriate permissions |
.github/workflows/integration-tests.yml |
Removed direct PR commenting and added PR number/artifact upload for use by the separate comment workflow |
src/ElectronNET.sln |
Added workflow files to solution items for better discoverability in the IDE |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
softworkz
force-pushed
the
submit_move_workflow
branch
from
02b4c6f to
54eac4b
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| upload-artifact: false | ||
| - name: Write PR Number to File | ||
| if: github.event_name == 'pull_request' | ||
| run: echo "$PR_NUMBER" > pr_number.txt |
There was a problem hiding this comment.
The PR_NUMBER environment variable may not be set when this step runs. The environment variable is set in the previous step (line 190), but the 'run' command in line 194 attempts to use it. Consider using the direct GitHub context value instead: echo "${{ github.event.pull_request.number }}" > pr_number.txt
| run: echo "$PR_NUMBER" > pr_number.txt | |
| run: echo "${{ github.event.pull_request.number }}" > pr_number.txt |
| PR_NUMBER=$(cat pr_number/pr_number.txt | grep -E '^[0-9]+$') | ||
| if [ -z "$PR_NUMBER" ]; then |
There was a problem hiding this comment.
The validation uses grep which will succeed if any line matches the pattern, but won't ensure the entire content is valid. Consider using a more robust validation: PR_NUMBER=$(cat pr_number/pr_number.txt | tr -d '\n' | grep -E '^[0-9]+$') to strip newlines first, or better yet: PR_NUMBER=$(cat pr_number/pr_number.txt); if ! [[ \"$PR_NUMBER\" =~ ^[0-9]+$ ]]; then echo \"Error: PR_NUMBER is not a valid integer.\"; exit 1; fi
| PR_NUMBER=$(cat pr_number/pr_number.txt | grep -E '^[0-9]+$') | |
| if [ -z "$PR_NUMBER" ]; then | |
| PR_NUMBER=$(cat pr_number/pr_number.txt | tr -d '\n') | |
| if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then |
| issue: ${{ env.PR_NUMBER }} | ||
|
|
||
| summary: true | ||
| pull-request: true |
There was a problem hiding this comment.
[nitpick] The pull-request: true parameter appears redundant when explicitly specifying issue: ${{ env.PR_NUMBER }}. This may cause confusion about whether the action should auto-detect the PR or use the provided issue number. Consider verifying the ctrf-io/github-test-reporter@v1 documentation to confirm if both parameters are needed or if one should be removed.
| pull-request: true |
| overwrite-comment: true | ||
| upload-artifact: false | ||
|
|
||
| pull-request-report: true |
There was a problem hiding this comment.
[nitpick] The pull-request-report: true parameter appears after upload-artifact: false and outside the main parameter block (lines 48-57), which may indicate it's redundant or incorrectly positioned given that pull-request reporting is already enabled via pull-request: true on line 52. Verify if this parameter is necessary or if it should be removed to avoid confusion.
| pull-request-report: true |
No description provided.