-
Notifications
You must be signed in to change notification settings - Fork 21
ci: fix ci #468
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: fix ci #468
Conversation
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.
Hey - 我发现了 1 个问题,并且有一些总体性的反馈:
- 条件性的
--with-core标志如果移到单独的步骤级if:条件中(例如拆成两个Test步骤,分别带和不带该 flag),会更易读也更不脆弱,而不是把 GitHub 表达式嵌入到 shell 命令里。 - 请再次确认从
on: push/pull_request.paths中移除.github/actions/setup/**和.github/actions/install-core/**路径过滤是否是对所有分支都刻意为之,因为这样会导致当只修改这些复合 action 时,CI 将不会运行。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- 条件性的 `--with-core` 标志如果移到单独的步骤级 `if:` 条件中(例如拆成两个 `Test`步骤,分别带和不带该 flag),会更易读也更不脆弱,而不是把 GitHub 表达式嵌入到 shell 命令里。
- 请再次确认从 `on: push/pull_request.paths` 中移除 `.github/actions/setup/**` 和 `.github/actions/install-core/**` 路径过滤是否是对所有分支都刻意为之,因为这样会导致当只修改这些复合 action 时,CI 将不会运行。
## Individual Comments
### Comment 1
<location> `.github/workflows/ci.yml:117` </location>
<code_context>
run: |
cargo run -pxtask test \
- ${{ matrix.runner != 'windows-11-arm' && --with-core }} \
+ ${{ matrix.runner != 'windows-11-arm' && '--with-core' }} \
-- --include-ignored
</code_context>
<issue_to_address>
**issue (bug_risk):** 条件表达式可能会插入 `true`/`false` 而不是空字符串,从而影响 cargo 命令。
在 GitHub Actions 中,像 `matrix.runner != 'windows-11-arm' && '--with-core'` 这样的 `&&` 链在条件不满足时会得到 `false`(布尔值),在 shell 中会被渲染为字符串 `false`,从而导致最终命令变成 `cargo run ... false -- --include-ignored`。如果你希望在这种情况下不传递任何参数,可以使用类似 `${{ matrix.runner != 'windows-11-arm' && '--with-core' || '' }}` 的三元风格模式,或者调整步骤逻辑,使该 flag 只在条件满足时才被添加。
</issue_to_address>帮我变得更有用!请对每条评论点 👍 或 👎,我会根据这些反馈改进后续的审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The conditional
--with-coreflag would be easier to read and less brittle if moved into a separate step-levelif:condition (e.g. twoTeststeps with and without the flag) rather than embedding a GitHub expression inside the shell command. - Double-check that removing the
.github/actions/setup/**and.github/actions/install-core/**path filters fromon: push/pull_request.pathsis intentional for all branches, since this will stop CI from running when only those composite actions change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The conditional `--with-core` flag would be easier to read and less brittle if moved into a separate step-level `if:` condition (e.g. two `Test` steps with and without the flag) rather than embedding a GitHub expression inside the shell command.
- Double-check that removing the `.github/actions/setup/**` and `.github/actions/install-core/**` path filters from `on: push/pull_request.paths` is intentional for all branches, since this will stop CI from running when only those composite actions change.
## Individual Comments
### Comment 1
<location> `.github/workflows/ci.yml:117` </location>
<code_context>
run: |
cargo run -pxtask test \
- ${{ matrix.runner != 'windows-11-arm' && --with-core }} \
+ ${{ matrix.runner != 'windows-11-arm' && '--with-core' }} \
-- --include-ignored
</code_context>
<issue_to_address>
**issue (bug_risk):** Conditional expression may interpolate `true`/`false` instead of an empty string, affecting the cargo command.
In GitHub Actions, an `&&` chain like `matrix.runner != 'windows-11-arm' && '--with-core'` evaluates to `false` (a boolean) when the condition fails, which is rendered as the string `false` in the shell, yielding `cargo run ... false -- --include-ignored`. If you want no argument in that case, use a ternary-style pattern like `${{ matrix.runner != 'windows-11-arm' && '--with-core' || '' }}` or adjust the step so the flag is only added conditionally.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #468 +/- ##
===========================================
- Coverage 69.25% 59.06% -10.20%
===========================================
Files 56 56
Lines 5601 5560 -41
Branches 5601 5560 -41
===========================================
- Hits 3879 3284 -595
- Misses 1430 2018 +588
+ Partials 292 258 -34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by Sourcery
调整 GitHub Actions 配置中的 CI 工作流触发条件,并修复条件测试命令
CI:
.github/actions中的 setup 步骤测试命令现在使用正确的 GitHub Actions 表达式语法,有条件地传递--with-core参数Original summary in English
Summary by Sourcery
Adjust CI workflow triggers and fix conditional test command in the GitHub Actions configuration
CI: