-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: Safe commenting on PRs #1729
Conversation
This generalizes and refactors what the QNS workflow did.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1729 +/- ##
==========================================
+ Coverage 89.42% 89.45% +0.02%
==========================================
Files 124 124
Lines 38887 38887
==========================================
+ Hits 34775 34785 +10
+ Misses 4112 4102 -10 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pulling this into separate actions! Overall looks good to me. Couple of nits / questions.
uses: actions/upload-artifact@v4 | ||
if: github.event_name == 'pull_request' | ||
- name: Export PR comment data | ||
if: ${{ github.event_name == 'pull_request' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each step in pr-comment-data-export
has this if
already. Can we remove it either here or in the action?
.github/workflows/check.yml
Outdated
printf "C:\\msys64\\usr\\bin" >> "$GITHUB_PATH" | ||
printf "C:\\msys64\\mingw64\\bin" >> "$GITHUB_PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When should we be using printf
over echo
? Note that there is another echo
below in this Windows
step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was because of a shellcheck
warning. The double backslashes aren't handled the same way in echo
in all bash'es apparently.
.github/workflows/qns-comment.yml
Outdated
if: > | ||
github.event.workflow_run.event == 'pull_request' && | ||
github.event.workflow_run.conclusion == 'failure' | ||
if: github.event.workflow_run.event == 'pull_request' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of qns, the comment should still only be printed on failure
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this needs to be restored - I was just trying to make it generate any output.
Merged, since can't really test this as a PR. Will continue on |
This generalizes and refactors what the QNS workflow did.