-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Pin CI image to windows-2025
#6148
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
WalkthroughUpdated GitHub Actions CI matrix in Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
246-249: Pin snippets_cpython matrix to windows-2025 (align Windows runner across CI)Replace windows-latest with windows-2025 in the snippets_cpython matrix. Remaining occurrences found:
- .github/workflows/ci.yaml:248 — matrix uses windows-latest (change this).
- .github/workflows/ci.yaml:122 — already uses windows-2025 (TODO to switch back on 2025-09-30).
- .github/workflows/release.yml:43 — runner: windows-latest (decide whether to pin).
Apply:
- os: [macos-latest, ubuntu-latest, windows-latest] + os: [macos-latest, ubuntu-latest, windows-2025]
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (2)
119-123: DRY the OS matrix via a YAML anchor to prevent drift.Optional: define the OS list once here and reuse in other jobs.
- os: - - macos-latest - - ubuntu-latest - - windows-2025 # TODO: Switch to `windows-latest` on 2025/09/30 + os: &os_matrix + - macos-latest + - ubuntu-latest + - windows-2025 # TODO: Switch to `windows-latest` on 2025/09/30Then in snippets_cpython:
- os: [macos-latest, ubuntu-latest, windows-latest] + os: *os_matrix
122-122: Track the revert date.Create a follow-up issue/reminder to switch back to
windows-lateston 2025-09-30 to avoid forgetting the TODO.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-11T09:35:43.571Z
Learnt from: ShaharNaveh
PR: RustPython/RustPython#5947
File: .github/workflows/ci.yaml:116-116
Timestamp: 2025-07-11T09:35:43.571Z
Learning: GitHub Actions timeout-minutes field supports expressions that evaluate to integers, such as `${{ contains(matrix.os, 'windows') && 40 || 30 }}`, which will conditionally set different timeout values based on the runner OS.
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-07-11T09:35:43.571Z
Learnt from: ShaharNaveh
PR: RustPython/RustPython#5947
File: .github/workflows/ci.yaml:116-116
Timestamp: 2025-07-11T09:35:43.571Z
Learning: GitHub Actions supports ternary-like expressions using `&&` and `||` operators in timeout-minutes. The syntax `${{ condition && value_if_true || value_if_false }}` is valid and commonly used, such as `${{ contains(matrix.os, 'windows') && 40 || 30 }}` which evaluates to 40 for Windows runners and 30 for others.
Applied to files:
.github/workflows/ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
🔇 Additional comments (1)
.github/workflows/ci.yaml (1)
119-123: Pin to windows-2025 looks good; timeout expression remains valid.The matrix change achieves the PR goal for rust_tests, and
contains(matrix.os, 'windows')still evaluates true forwindows-2025, so timeouts behave as before.
This reverts commit 43d643a.
This reverts commit 43d643a.
Right now our tests are patched for windows-2025, and sometimes our runner gets the
windows-2022image which causes the CI to fail.For more info see see:
https://github.blog/changelog/2025-07-31-github-actions-new-apis-and-windows-latest-migration-notice/#windows-latest-image-label-migration
Summary by CodeRabbit