-
Notifications
You must be signed in to change notification settings - Fork 0
ci: matrix builds with native ARM64 runners and PR test comments #62
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
Conversation
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
Comment |
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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/build.yml:
- Around line 143-208: The test result JSON needs a job status field and the
comment logic must consider it: update the step that writes test results to
include a stable field (e.g., "status" or "job.status") reflecting the job
outcome, then modify the GitHub Script that reads files (the files loop using
const files = ... and data = JSON.parse(...)) to mark allPassed = false when
either data.failed > 0 OR data.status !== 'success'; also adjust the
status/emoji logic (the const status = ... line) to use data.status alongside
failed so compile/setup failures show ❌ and the commentBody accurately reports
failures when tests never ran. Ensure you reference the same field name in both
the producer and consumer of the JSON.
- Around line 56-89: The test runner currently discards stdout/stderr with "|
Out-Null", losing output needed for PR comments; update the block that invokes
$_.FullName to capture the command output into a variable (e.g., $output = &
$_.FullName 2>&1), then use $LASTEXITCODE as before to determine pass/fail and
append the captured output to $testResults (include it alongside the
"PASSED"/"FAILED" message and exit code) so downstream post-test-results job can
post detailed logs; ensure the catch block also captures and appends $_ (the
exception) and any $output to $testResults for failed runs.
In @.github/workflows/release.yml:
- Around line 158-162: The workflow uses git log on HEAD to build
mergeCommitBody (variable mergeCommitBody) which is invalid when a PR is closed
without merging; add a guard that checks github.event.pull_request.merged ==
true before running release-note generation steps and replace the HEAD lookup
with the actual merge commit SHA (github.event.pull_request.merge_commit_sha)
when populating mergeCommitBody so the workflow always reads the correct merge
commit message.
|
@copilot In @.github/workflows/build.yml:
In @.github/workflows/release.yml:
|
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
Fixed in commit 8906b05: build.yml:
release.yml:
|
…d errors Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/build.yml:
- Around line 197-205: The logic currently hides test counts when status !==
'success'; modify the branch around commentBody construction (variables: status,
total, passed, failed, arch, commentBody) so the "Job failed - Tests may not
have run..." message is only added when total === 0 (compile/setup failure). In
practice, change the condition to check both status and total (e.g., if (status
!== 'success' && total === 0) show the setup-failure message) and ensure the
else branch prints the `${passed}/${total} tests passed` line even when status
=== 'failure' but total > 0 so failed test runs still display counts.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
160-166: Good use of merge commit SHA, but consider adding defensive validation.The change correctly uses
merge_commit_shainstead ofHEADto retrieve the actual merge commit body. With the merged guard at line 17, this SHA should always be valid.However, adding a defensive check for an empty SHA would make the workflow more robust against edge cases:
🛡️ Optional defensive validation
$mergeCommitSha = "${{ github.event.pull_request.merge_commit_sha }}" Write-Output "DEBUG: Merge commit SHA: $mergeCommitSha" - $mergeCommitBody = git log -1 --pretty=format:"%b" $mergeCommitSha + $mergeCommitBody = "" + if (-not [string]::IsNullOrWhiteSpace($mergeCommitSha)) { + $mergeCommitBody = git log -1 --pretty=format:"%b" $mergeCommitSha + } Write-Output "DEBUG: Merge commit body: $mergeCommitBody"
CI Workflow Upgrades
Plan
Changes Made
release.yml:
github.event.pull_request.merged == true)github.event.pull_request.merge_commit_shainstead of HEAD for correct merge commitbuild.yml:
windows-11-armrunner for native compilation and testing (no cross-compilation needed!)fail-fast: false$outputoutside try blockOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
Chores
✏️ Tip: You can customize this high-level summary in your review settings.