ci: treat skipped build or validation as success#1148
Conversation
|
Error: Could not generate a valid Mermaid diagram after multiple attempts. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates the CI workflow to treat skipped HLSL validation as a success by adding a dedicated step that checks for HLSL changes and conditionally skips related validation steps.
- Added a new "Check if HLSL validation needed" step to set a skip flag based on hlsl-should-build.
- Removed redundant conditions in the workflow trigger and applied the skip flag to subsequent steps.
| @@ -182,21 +180,37 @@ jobs: | |||
| file: ".github/configs/shader-validation-vr.yaml" | |||
| fail-fast: false # Let both configs run to completion for full output | |||
There was a problem hiding this comment.
Consider consolidating the repeated condition 'if: steps.check-hlsl.outputs.skip != 'true'' into a job-level condition or shared variable to reduce redundancy and enhance maintainability.
| fail-fast: false # Let both configs run to completion for full output | |
| fail-fast: false # Let both configs run to completion for full output | |
| env: | |
| SKIP_VALIDATION: ${{ steps.check-hlsl.outputs.skip }} |
| - name: Check if HLSL validation needed | ||
| id: check-hlsl | ||
| run: | | ||
| if [ "${{ needs.check-changes.outputs.hlsl-should-build }}" != "true" ]; then |
There was a problem hiding this comment.
Ensure that using ${{ needs.check-changes.outputs.hlsl-should-build }} in the shell condition consistently provides the expected value at runtime; consider additional logging if the variable might be unset or differ in format.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build.yaml (2)
183-194: Introduce initial HLSL validation gating step
The newcheck-hlslstep correctly inspectsneeds.check-changes.outputs.hlsl-should-buildand emits askipflag, ensuring subsequent steps run only when needed. For clarity, consider renaming the output fromskipto something likeskip_hlsl_validationto avoid ambiguity.
196-197: DRY up repeated skip conditions across validation steps (optional)
Steps aftercheck-hlslall use the same condition:steps.check-hlsl.outputs.skip != 'true'. You could reduce duplication with a YAML anchor:+ x-skip-hlsl: &skip_hlsl steps.check-hlsl.outputs.skip != 'true' steps: - name: Checkout code - if: steps.check-hlsl.outputs.skip != 'true' + if: *skip_hlsl uses: actions/checkout@v4 - uses: ilammy/msvc-dev-cmd@v1.10.0 - if: steps.check-hlsl.outputs.skip != 'true' + if: *skip_hlsl # ...This makes future updates easier by centralizing the skip logic.
Also applies to: 203-205, 206-209, 212-214, 223-225, 231-233, 237-239, 242-244, 247-249
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml(3 hunks)
🔇 Additional comments (1)
.github/workflows/build.yaml (1)
164-170: Decoupled job triggering and execution gating for shader-validation
The job-levelif:is now simplified to always run on non-draft PRs (or manual runs withvalidate-shaders:true), moving HLSL-specific checks into dedicated step logic. This separation enhances readability and maintainability.
|
Hmm, may need to handle the cpp side better too. If no cpp changes, we also don't post a build. |
alandtse
changed the title
This reverts commit dafb4b9.
|
Actually, we always have to build cpp, otherwise we don't have anything to post. |
* ci: treat skipped hlsl validation as success * chore: trigger cpp validation * ci: allow cpp success without changes * revert: "chore: trigger cpp validation" This reverts commit dafb4b9.
) * ci: treat skipped hlsl validation as success * chore: trigger cpp validation * ci: allow cpp success without changes * revert: "chore: trigger cpp validation" This reverts commit dafb4b9.
) * ci: treat skipped hlsl validation as success * chore: trigger cpp validation * ci: allow cpp success without changes * revert: "chore: trigger cpp validation" This reverts commit dafb4b9.
Summary by CodeRabbit