Conversation
915370f to
ea95774
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3664 +/- ##
=======================================
Coverage 81.32% 81.32%
=======================================
Files 620 620
Lines 39086 39086
Branches 6375 6351 -24
=======================================
Hits 31787 31787
- Misses 6316 6329 +13
+ Partials 983 970 -13 ☔ View full report in Codecov by Sentry. |
ea95774 to
414661d
Compare
414661d to
f84959b
Compare
|
Given that servers have been upgraded, it probably makes sense to now review and merge this. |
| strategy: | ||
| matrix: | ||
| os: ["ubuntu-22.04"] | ||
| os: ["ubuntu-24.04"] |
There was a problem hiding this comment.
🚩 chromium-browser references may need updating for Ubuntu 24.04 compatibility
The pre-build report steps in build-and-test.yml:65-66 and build-and-test.yml:180-181 run which chromium-browser and chromium-browser --version under set -xueo pipefail. On standard Ubuntu 24.04, chromium-browser is a transitional package that redirects to the snap chromium package, and the binary name is typically chromium rather than chromium-browser. GitHub Actions runner images for ubuntu-24.04 may provide chromium-browser through a third-party PPA (e.g., chromium-team/stable), which would make the command available. However, this depends on the runner image's specific configuration and PPA support for Noble (24.04). If chromium-browser is not available, the step will fail due to set -e, blocking the entire CI pipeline. Additionally, the test:gha npm script at src/SIL.XForge.Scripture/ClientApp/package.json:17 uses xForgeChromiumHeadless (based on ChromiumHeadless from karma-chrome-launcher), which also looks for chromium-browser in PATH. Worth verifying these commands work on the new runner image.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I don't think it's a problem. And the E2E build passed.
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 3 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Nateowami).
| strategy: | ||
| matrix: | ||
| os: ["ubuntu-22.04"] | ||
| os: ["ubuntu-24.04"] |
There was a problem hiding this comment.
I don't think it's a problem. And the E2E build passed.
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 1 comment and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Nateowami).
f84959b to
6ced90c
Compare
| npm_version: "11.11.0" | ||
| # When bumping OS version, the server will need to authorize the new default rsync args. | ||
| os: "ubuntu-22.04" | ||
| os: "ubuntu-24.04" |
There was a problem hiding this comment.
🚩 rsync compatibility warning: server may need reconfiguration for ubuntu-24.04
The existing comments at release-live.yml:120 and release-qa.yml:141 explicitly warn: "When bumping OS version, the server will need to authorize the new default rsync args." This PR bumps the OS version from ubuntu-22.04 to ubuntu-24.04, which is exactly the scenario the comment warns about. Ubuntu 24.04 ships rsync 3.2.7+ which may use different default arguments (e.g., different compression or checksum algorithms like xxhash) compared to Ubuntu 22.04's rsync. If the deployment server uses rrsync or other restrictions on allowed rsync arguments, the deploy step in release.yml:163 (scripts/build-and-ship) could fail. The server-side configuration should be verified before deploying from an ubuntu-24.04 runner.
Was this helpful? React with 👍 or 👎 to provide feedback.
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Nateowami).
|
(I added do-not-merge tag related to a comment I made about not merging until after a coming production release.) |
6ced90c to
44e3440
Compare
This PR exists to test what happens when we start running builds on Ubuntu 24.04. It exists as a test, rather than something we actually intend to merge.
This change is