refactor(ci): force query-planner CI if engine changes#2165
Conversation
…hanges - Renamed job from 'check-label' to 'check-label-and-engine-changes'. - Added a step to check for changes in the engine version. - Updated dependencies in subsequent jobs to reflect the new job name and outputs.
WalkthroughRenames job to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
…anner CI workflow
…-planner CI workflow
…n compatibility to test it
|
❌ Internal Query Planner CI failed to run. |
…pull request base SHA for go.mod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/query-planner-ci.yaml (2)
135-141: Gate the job earlier to avoid spinning a runner when not needed.The inner step is already gated, but the job runs unconditionally (always()). Consider moving the OR condition to the job-level if and letting the step run unconditionally.
Example:
- build_test: # This job is use to lock the merge of the PR if anything fails in the query planner + build_test: # This job locks the merge of the PR if anything fails in the query planner runs-on: ubuntu-latest - if: ${{ always() }} + if: ${{ needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true' }} needs: [ build-router, build-cli, cli-release-url, check-label-and-engine-changes ] @@ - - uses: convictional/trigger-workflow-and-wait@v1.6.5 - if: ${{ needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true' }} + - uses: convictional/trigger-workflow-and-wait@v1.6.5This saves a runner when the gate is false.
215-217: Typo: step name says “Comment PR on failure” in the success job.Minor, but confusing in logs. Rename that step to “Comment PR on success”.
Suggested change (outside selected lines, line 219):
- - name: Comment PR on failure + - name: Comment PR on success
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/query-planner-ci.yaml(7 hunks)
🔇 Additional comments (4)
.github/workflows/query-planner-ci.yaml (4)
13-13: Good call adding the workflow file to the path filters.Ensures the workflow re-runs when the workflow itself changes. LGTM.
158-161: Gating logic looks correct.Runs only when label is present or engine changed, regardless of previous job results (thanks to always()). LGTM.
196-198: Failure comment gating is correct.The job triggers only on the gated path and when the workflow has failed. Good wiring with needs and failure().
25-30: No stale workflow references found—rename is consistentI searched all GitHub workflow files for any occurrences of the old
check-labeljob ID (excluding the newly introducedcheck-label-and-engine-changes) and found none. The job rename and addedengine_changedoutput are correctly reflected everywhere in.github/workflows/query-planner-ci.yaml, and there are no lingering references to clean up.– No further action needed; the rename and new output usage are safe to merge.
…o 0 and adding Go setup step
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/query-planner-ci.yaml (2)
31-34: Checkout depth fixed to avoid missing base commit.Switching to
fetch-depth: 0resolves shallow-fetch issues when accessing the PR base SHA. Nice catch.
52-64: Engine-change detector can give false results: use .Version instead of .Sum and harden the script.
.Sumdepends on go.sum and will be empty when using an alternate-modfilethat has no matching go.sum (we write only/tmp/base.go.mod). This can cause empty/unequal comparisons and spurious triggers or misses. Comparing.Versionis robust and side-effect free. Also add strict shell options and quote$GITHUB_OUTPUT.Apply:
- - name: Check for changes of engine + - name: Check for changes of engine id: check_engine_changes working-directory: router run: | - current_engine_version=$(go list -f '{{.Sum}}' -m github.com/wundergraph/graphql-go-tools/v2) - git -C "$GITHUB_WORKSPACE" show ${{ github.event.pull_request.base.sha }}:router/go.mod > /tmp/base.go.mod - previous_engine_version=$(go list -f '{{.Sum}}' -modfile /tmp/base.go.mod -m github.com/wundergraph/graphql-go-tools/v2) - if [ "$current_engine_version" != "$previous_engine_version" ]; then - echo "engine_changed=true" >> $GITHUB_OUTPUT - else - echo "engine_changed=false" >> $GITHUB_OUTPUT - fi + set -euo pipefail + curr_ver="$(go list -m -f '{{.Version}}' github.com/wundergraph/graphql-go-tools/v2)" + git -C "$GITHUB_WORKSPACE" show ${{ github.event.pull_request.base.sha }}:router/go.mod > /tmp/base.go.mod + prev_ver="$(go list -m -modfile=/tmp/base.go.mod -f '{{.Version}}' github.com/wundergraph/graphql-go-tools/v2)" + if [ "$curr_ver" != "$prev_ver" ]; then + echo "engine_changed=true" >> "$GITHUB_OUTPUT" + else + echo "engine_changed=false" >> "$GITHUB_OUTPUT" + fi
🧹 Nitpick comments (5)
.github/workflows/query-planner-ci.yaml (5)
35-39: Nit: remove trailing spaces and keep action pinning consistent.
- There’s trailing whitespace on Line 35 flagged by yamllint.
Apply:
- - name: Setup Go + - name: Setup Go
140-146: Guard the external trigger for forked PRs lacking org secrets.
convictional/trigger-workflow-and-waitneedssecrets.GH_TOKEN_CELESTIAL_TRIGGER. For PRs from forks, this secret is unavailable and the step will fail even if gating conditions pass. Add a secrets check to the step’sifto avoid noise, and let the surrounding jobs post the generic failure comment if desired.- - uses: convictional/trigger-workflow-and-wait@v1.6.5 - if: ${{ needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true' }} + - uses: convictional/trigger-workflow-and-wait@v1.6.5 + if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && secrets.GH_TOKEN_CELESTIAL_TRIGGER != '' }}
163-166: Also gate get_pr_url on secrets availability to prevent a hard failure on forks.This job uses
secrets.GH_TOKEN_CELESTIAL_TRIGGERwithinactions/github-scriptto access artifacts. Add the same guard.- if: ${{ always() && (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') }} + if: ${{ always() && (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && secrets.GH_TOKEN_CELESTIAL_TRIGGER != '' }}
201-203: Guard failure comment path for forks with missing secret.Prevents this job from failing when the token isn’t injected on forked PRs.
- if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && failure() }} + if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && failure() && secrets.GH_TOKEN_CELESTIAL_TRIGGER != '' }}
220-222: Guard success comment path for forks with missing secret.Same rationale as failure path.
- if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && success() }} + if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && success() && secrets.GH_TOKEN_CELESTIAL_TRIGGER != '' }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/query-planner-ci.yaml(7 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/query-planner-ci.yaml
[error] 35-35: trailing spaces
(trailing-spaces)
⏰ 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). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
.github/workflows/query-planner-ci.yaml (2)
13-13: Good addition: include the workflow file in its own path filter.Prevents accidental misses when this workflow changes. LGTM.
25-29: Job rename + new output wiring looks consistent.
check-label-and-engine-changesexposes bothhas_labelandengine_changed. Downstream jobs in this file reference the new job name and outputs correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/query-planner-ci.yaml (1)
52-63: Harden engine change detection: avoid using .Sum, enable strict bash, and prevent network flakiness.Current approach uses
{{.Sum}}which depends on the presence of go.sum entries and may trigger network resolution (esp. with-modfilepointing to a temp go.mod without an accompanying go.sum). This can lead to false negatives or step failures. Prefer comparing{{.Version}}, run withset -euo pipefail, and quote$GITHUB_OUTPUT.Proposed diff:
- - name: Check for changes of engine + - name: Check for changes of engine id: check_engine_changes working-directory: router run: | - current_engine_version=$(go list -f '{{.Sum}}' -m github.com/wundergraph/graphql-go-tools/v2) - git -C "$GITHUB_WORKSPACE" show ${{ github.event.pull_request.base.sha }}:router/go.mod > /tmp/base.go.mod - previous_engine_version=$(go list -f '{{.Sum}}' -modfile /tmp/base.go.mod -m github.com/wundergraph/graphql-go-tools/v2) - if [ "$current_engine_version" != "$previous_engine_version" ]; then - echo "engine_changed=true" >> $GITHUB_OUTPUT - else - echo "engine_changed=false" >> $GITHUB_OUTPUT - fi + set -euo pipefail + # Obtain current version from PR head without mutating workspace + curr_ver="$(go list -m -f '{{.Version}}' github.com/wundergraph/graphql-go-tools/v2)" + # Read base go.mod directly (no checkout) and resolve the version against it + git -C "$GITHUB_WORKSPACE" show ${{ github.event.pull_request.base.sha }}:router/go.mod > /tmp/base.go.mod + # Note: using {{.Version}} avoids reliance on go.sum availability + prev_ver="$(go list -m -modfile=/tmp/base.go.mod -f '{{.Version}}' github.com/wundergraph/graphql-go-tools/v2)" + if [ -z "${curr_ver}" ] || [ -z "${prev_ver}" ]; then + echo "engine_changed=false" >> "$GITHUB_OUTPUT" + elif [ "$curr_ver" != "$prev_ver" ]; then + echo "engine_changed=true" >> "$GITHUB_OUTPUT" + else + echo "engine_changed=false" >> "$GITHUB_OUTPUT" + fiThis keeps the step side-effect free and resilient in CI.
🧹 Nitpick comments (6)
.github/workflows/query-planner-ci.yaml (6)
31-39: Checkout depth fixed; add trailing-space cleanup.
- Using
fetch-depth: 0resolves the base-SHA checkout issues from earlier feedback.- Minor: Line 35 has trailing spaces flagged by yamllint.
Apply this minimal diff:
- - name: Setup Go + - name: Setup GoIf you want to be extra tidy, run yamllint locally for the file.
40-43: Standardize on actions/github-script v7.You already use v7 later (Line 170). For consistency and to pick up the latest fixes, bump this one as well.
- uses: actions/github-script@v6 + uses: actions/github-script@v7
4-12: Deduplicate path entries (minor).
connect/**/*appears twice in the pull_request.paths list. Harmless but noisy.- 'connect/**/*' - '.github/workflows/cli-ci.yaml' - 'router/**/*' - - 'connect/**/*' - '.github/workflows/router-ci.yaml'
146-161: Gating condition looks right; consider avoiding the ':latest' fallback for determinism.Conditionally triggering when either label is present or engine changed is perfect. For the payload, defaulting router image to
:latestcan introduce flakiness across runs.Options:
- Default to a stable tag (e.g.,
:main) or a known digest.- Alternatively, if no image was built, pass an explicit "use-default" flag and let the downstream workflow resolve a deterministic image.
Would you like a follow-up patch to use
:maininstead?
170-170: Nice upgrade to actions/github-script v7 here.As noted above, consider bumping the earlier usage to v7 for consistency.
220-222: Success comment gating matches; fix step label nit.Everything gates correctly. Minor nit: in this success job, the step name at Line 224 says "Comment PR on failure".
- - name: Comment PR on failure + - name: Comment PR on success
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/query-planner-ci.yaml(7 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/query-planner-ci.yaml
[error] 35-35: trailing spaces
(trailing-spaces)
⏰ 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). (3)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
.github/workflows/query-planner-ci.yaml (5)
13-13: Good call adding the workflow to its own path filter.Ensures CI reruns when this file changes. Looks correct.
25-30: Job rename and outputs wiring look consistent.
check-label-and-engine-changesexposeshas_labelandengine_changedas strings ("true"/"false") and downstream jobs reference them correctly.
140-140: Downstream needs updated correctly.Adding
check-label-and-engine-changestoneedsis correct so outputs are available for gating.
163-166: LGTM on gating get_pr_url to the same OR condition.Prevents unnecessary artifact fetches when the gate is closed.
201-203: Failure comment gating matches the new OR condition.Looks correct and will only comment when the gate is open and the workflow failed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/query-planner-ci.yaml (1)
31-34: Checkout depth fixed — resolves prior shallow history concern.Using fetch-depth: 0 guarantees the base commit exists for comparisons.
🧹 Nitpick comments (3)
.github/workflows/query-planner-ci.yaml (3)
35-39: Go toolchain setup is in the right place; remove trailing spaces to satisfy linters.Yamllint flagged trailing spaces at Line 35. Drop the trailing spaces to avoid CI noise; keep the rest as-is.
Apply this minimal diff:
- - name: Setup Go + - name: Setup Go
40-52: Standardize on actions/github-script v7 (Node20) for consistency and future-proofing.You already use v7 later (Line 172). Upgrade here too to avoid mixed runtimes.
- uses: actions/github-script@v6 + uses: actions/github-script@v7Optional: in the script call, pass per_page: 100 to avoid edge-cases with repos that might have >30 labels.
- const labels = await github.rest.issues.listLabelsOnIssue({ + const labels = await github.rest.issues.listLabelsOnIssue({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: context.issue.number + issue_number: context.issue.number, + per_page: 100 });
222-224: Success comment job is correctly gated; minor step name nit.Inside this job, the step at Lines 226-231 is named “Comment PR on failure”. Rename to “Comment PR on success” for clarity.
- - name: Comment PR on failure + - name: Comment PR on success
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/query-planner-ci.yaml(7 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/query-planner-ci.yaml
[error] 35-35: trailing spaces
(trailing-spaces)
⏰ 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). (2)
- GitHub Check: check-label-and-engine-changes
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
.github/workflows/query-planner-ci.yaml (7)
13-13: Good addition: include the workflow itself in the trigger paths.Ensures the workflow re-runs when edited. Looks correct.
25-30: Job rename + new output are wired correctly.Outputs use the right step IDs and are referenced downstream; no issues spotted.
147-157: Double-check secret availability for forked PRs.This step needs GH_TOKEN_CELESTIAL_TRIGGER. For pull_request events from forks, repo secrets are not exposed by default. Your gating relies on the label or engine change; if those are true on a forked PR, this step may still not have the secret and will fail.
If forks are in scope, either:
- run this workflow only on pull_request_target with careful hardening; or
- add an additional guard checking github.event.pull_request.head.repo.full_name == github.repository (similar to build-router), or
- ensure maintainers-only labels are applied only on same-repo PRs.
Would you like me to draft a guard condition?
142-149: Gating build_test on label OR engine change is correct; nice use of step-level if.The job runs always but the external workflow is only triggered when required. Clean.
161-163: Sensible default for router image.Fallback to ghcr.io/wundergraph/cosmo/router:latest is appropriate.
165-168: Propagation and OR-logic in get_pr_url look good.Job executes only when the gate signals true. All good.
203-205: Failure comment job is correctly gated.Runs only when the gate is true and build/test path fails. Good wiring.
There was a problem hiding this comment.
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/query-planner-ci.yaml (1)
128-130: Bug: this job readsneeds.filter-changesbut doesn’t declare a dependency
cli-release-urlreferencesneeds.filter-changes.outputs.cli_changedin itsif, but the job has noneeds. Add it, otherwise the expression is undefined and can fail.- cli-release-url: + cli-release-url: runs-on: ubuntu-latest - if: ${{ needs.filter-changes.outputs.cli_changed != 'true' }} + needs: [ filter-changes ] + if: ${{ needs.filter-changes.outputs.cli_changed != 'true' }}
♻️ Duplicate comments (2)
.github/workflows/query-planner-ci.yaml (2)
31-34: Resolved: shallow checkout issue fixed with fetch-depth: 0Switching to
fetch-depth: 0ensures the PR base SHA is present for comparisons. Nice.
52-66: Harden engine change detection: compare versions, set strict shell, avoid workspace writes, and quote GITHUB_OUTPUTCurrent logic compares
.Sum, writesbase.go.modinto the repo workspace, and doesn’t set strict shell or quote$GITHUB_OUTPUT. Prefer comparing module versions and reading the base file viagit showinto/tmp, with strict shell options.- name: Check for changes of engine id: check_engine_changes working-directory: router run: | - current_engine_version=$(go list -f '{{.Sum}}' -m github.com/wundergraph/graphql-go-tools/v2) - git show ${{ github.event.pull_request.base.sha }}:router/go.mod > base.go.mod - previous_engine_version=$(go list -f '{{.Sum}}' -modfile base.go.mod -m github.com/wundergraph/graphql-go-tools/v2) - if [ "$current_engine_version" != "$previous_engine_version" ]; then - echo "engine_changed=true" >> $GITHUB_OUTPUT - echo "Engine has been changed from $previous_engine_version to $current_engine_version" - else - echo "engine_changed=false" >> $GITHUB_OUTPUT - echo "Engine has not been changed" - fi + set -euo pipefail + # Current version from PR head + curr_ver="$(go list -m -f '{{`{{.Version}}`}}' github.com/wundergraph/graphql-go-tools/v2 || true)" + # Read base go.mod without mutating the workspace + git show ${{ github.event.pull_request.base.sha }}:router/go.mod > /tmp/base.go.mod + # Previous version resolved against the base go.mod + prev_ver="$(go list -m -modfile=/tmp/base.go.mod -f '{{`{{.Version}}`}}' github.com/wundergraph/graphql-go-tools/v2 || true)" + if [ "${curr_ver:-}" != "${prev_ver:-}" ]; then + echo "engine_changed=true" >> "$GITHUB_OUTPUT" + echo "Engine has been changed from ${prev_ver:-<none>} to ${curr_ver:-<none>}" + else + echo "engine_changed=false" >> "$GITHUB_OUTPUT" + echo "Engine has not been changed" + fiNotes:
.Versionis a clearer comparison than.Sumand avoids proxy/sumdb nuances.- The backtick-wrapped template prevents Actions expression parsing of
{{ }}in YAML.- Quoting
$GITHUB_OUTPUTavoids edge cases with spaces.
🧹 Nitpick comments (6)
.github/workflows/query-planner-ci.yaml (6)
8-12: Minor: duplicateconnect/**/*path
connect/**/*appears twice in the paths list (Line 8 and Line 11). It’s harmless but noisy. Remove one entry.- 'connect/**/*' - - 'connect/**/*'
25-30: Expose a singleshould_runoutput to simplify downstream gatingOutputs look good. Consider also publishing a combined boolean so jobs can check a single flag instead of repeating the OR everywhere.
outputs: has_label: ${{ steps.check_label.outputs.has_label }} engine_changed: ${{ steps.check_engine_changes.outputs.engine_changed }} + should_run: ${{ steps.check_label.outputs.has_label == 'true' || steps.check_engine_changes.outputs.engine_changed == 'true' }}Then downstream conditions can use
needs.check-label-and-engine-changes.outputs.should_run == 'true'.
35-39: Trailing spaces and consistency nit
- YAMLlint flagged trailing spaces on Line 35 (“Setup Go ”). Trim to keep lint clean.
- Everything else here (go-version-file + cache) looks correct.
- - name: Setup Go + - name: Setup Go
41-43: Align github-script to v7 (used elsewhere in this workflow)You’re already on
actions/github-script@v7later (Line 172). Bump this step for consistency and to stay current.- uses: actions/github-script@v6 + uses: actions/github-script@v7
142-149: Optional: move gating to the job-levelifto save minutesThe step-level gate works, but you can skip the whole
build_testjob when not needed by moving the OR condition to the job’sif. Keepalways()only if you explicitly want the job to appear and succeed when skipped.- build_test: # This job is use to lock the merge of the PR if anything fails in the query planner + build_test: # This job is used to lock the merge of the PR if anything fails in the query planner runs-on: ubuntu-latest - if: ${{ always() }} + if: ${{ needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true' }} needs: [ build-router, build-cli, cli-release-url, check-label-and-engine-changes ] @@ - - uses: convictional/trigger-workflow-and-wait@v1.6.5 - if: ${{ needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true' }} + - uses: convictional/trigger-workflow-and-wait@v1.6.5If you adopt the earlier
should_runoutput, the jobifbecomes:
if: ${{ needs.check-label-and-engine-changes.outputs.should_run == 'true' }}.
161-161: Defaulting to:latestis nondeterministicFalling back to
ghcr.io/wundergraph/cosmo/router:latestcan cause flaky runs due to drift. Prefer a pinned tag/digest, or fail fast whenimage_refis empty for org members and only fall back for external contributors.- "router": "${{ needs.build-router.outputs.image_ref || 'ghcr.io/wundergraph/cosmo/router:latest' }}", + "router": "${{ needs.build-router.outputs.image_ref || 'ghcr.io/wundergraph/cosmo/router:main' }}",Or gate the fallback explicitly on the external contributor condition you already use in
build-router.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/query-planner-ci.yaml(7 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/query-planner-ci.yaml
[error] 35-35: trailing spaces
(trailing-spaces)
⏰ 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). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
.github/workflows/query-planner-ci.yaml (4)
13-13: Good call: include self-change to always rerun on workflow editsAdding
.github/workflows/query-planner-ci.yamltoon.pull_request.pathsensures this workflow self-triggers when modified. LGTM.
165-169: LGTM: correct gating and dependency for PR URL retrieval
get_pr_urlwaits onbuild_testand applies the same OR condition. Good wiring.
203-205: LGTM: failure comment is gated on label-or-engine AND failure()The condition prevents noisy comments when the workflow didn’t need to run. Looks good.
222-224: LGTM: success comment mirrors failure gatingConsistent gating with the OR condition and success(). All good.
There was a problem hiding this comment.
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/query-planner-ci.yaml (1)
147-156: Critical: secrets not available on forked PRs — gate the trigger step to internal PRstrigger-workflow-and-wait uses secrets.GH_TOKEN_CELESTIAL_TRIGGER. GitHub does not pass secrets to pull_request workflows from forks. If an external PR bumps the engine (engine_changed == 'true'), this step will run and fail, blocking the PR. Gate it like build-router does.
- - uses: convictional/trigger-workflow-and-wait@v1.6.5 - if: ${{ needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true' }} + - uses: convictional/trigger-workflow-and-wait@v1.6.5 + if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && github.event.pull_request.head.repo.full_name == github.repository }} id: trigger_workflowFollow through by mirroring the same repo-gating on the downstream jobs that also rely on that secret:
- if: ${{ always() && (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') }} + if: ${{ always() && (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && github.event.pull_request.head.repo.full_name == github.repository }}(lines 167)
- if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && failure() }} + if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && github.event.pull_request.head.repo.full_name == github.repository && failure() }}(lines 204)
- if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && success() }} + if: ${{ (needs.check-label-and-engine-changes.outputs.has_label == 'true' || needs.check-label-and-engine-changes.outputs.engine_changed == 'true') && github.event.pull_request.head.repo.full_name == github.repository && success() }}(lines 223)
♻️ Duplicate comments (1)
.github/workflows/query-planner-ci.yaml (1)
31-34: Resolved: ensure PR base SHA availability by using fetch-depth: 0Switching checkout to fetch-depth: 0 addresses prior concerns about missing base commits in long-lived PRs. Nice.
🧹 Nitpick comments (5)
.github/workflows/query-planner-ci.yaml (5)
35-39: Fix trailing spaces and keep YAML lint cleanYAMLlint flags trailing spaces on Line 35 (“Setup Go ”). Trim to avoid CI lint noise.
Apply this diff:
- - name: Setup Go + - name: Setup Go
42-42: Upgrade actions/github-script to v7 for consistency and Node 20You already use @v7 later; bump this step too for consistency and future-proofing.
- uses: actions/github-script@v6 + uses: actions/github-script@v7
52-65: Harden the engine-change check: strict shell, temp file, quote GITHUB_OUTPUTFunctionally fine, but a few robustness tweaks reduce flaky failures:
- Add set -euo pipefail to fail fast.
- Write the base go.mod into $RUNNER_TEMP (avoid polluting the workspace).
- Quote "$GITHUB_OUTPUT".
- run: | - current_engine_version=$(go list -f '{{.Version}}' -m github.com/wundergraph/graphql-go-tools/v2) - git show ${{ github.event.pull_request.base.sha }}:router/go.mod > base.go.mod - previous_engine_version=$(go list -f '{{.Version}}' -modfile base.go.mod -m github.com/wundergraph/graphql-go-tools/v2) - if [ "$current_engine_version" != "$previous_engine_version" ]; then - echo "engine_changed=true" >> $GITHUB_OUTPUT - echo "Engine has been changed from $previous_engine_version to $current_engine_version" - else - echo "engine_changed=false" >> $GITHUB_OUTPUT - echo "Engine has not been changed" - fi + run: | + set -euo pipefail + curr_ver="$(go list -m -f '{{.Version}}' github.com/wundergraph/graphql-go-tools/v2)" + base_mod="${RUNNER_TEMP}/router.base.go.mod" + git show ${{ github.event.pull_request.base.sha }}:router/go.mod > "${base_mod}" + prev_ver="$(go list -m -modfile="${base_mod}" -f '{{.Version}}' github.com/wundergraph/graphql-go-tools/v2)" + if [ "${curr_ver:-}" != "${prev_ver:-}" ]; then + echo "engine_changed=true" >> "$GITHUB_OUTPUT" + echo "Engine has been changed from ${prev_ver:-<none>} to ${curr_ver:-<none>}" + else + echo "engine_changed=false" >> "$GITHUB_OUTPUT" + echo "Engine has not been changed" + fi
8-12: Remove duplicate path entryconnect/**/* appears twice in on.pull_request.paths. Harmless but noisy; remove the duplicate.
- 'router/**/*' - - 'connect/**/*' - '.github/workflows/router-ci.yaml'
226-231: Typo: step name says failure in success blockMinor nit: the success comment step is named “Comment PR on failure”.
- - name: Comment PR on failure + - name: Comment PR on success
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/query-planner-ci.yaml(7 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/query-planner-ci.yaml
[error] 35-35: trailing spaces
(trailing-spaces)
⏰ 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). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
.github/workflows/query-planner-ci.yaml (4)
25-30: Solid job rename and output wiringRenaming to check-label-and-engine-changes and exposing engine_changed as a job output is coherent and correctly referenced by downstream jobs.
159-163: Confirm fallback router image referenceThe fallback router image is ghcr.io/wundergraph/cosmo/router:latest. Please confirm this tag exists and is the intended default for query-planner tests.
If needed, I can provide a small script using gh api to list package versions.
81-87: Verify filter scope: router changed when demo/ changes*router filter includes demo//*, which will mark router_changed=true for demo-only changes. If that’s intentional, ignore. If not, drop demo//* from the router filter.
167-167: Please verify the upstream default branch of cosmo-celestial
The commandgh repo view wundergraph/cosmo-celestial --json defaultBranchRef -q '.defaultBranchRef.name'returned a “Repository not found” error. Manually confirm whether the default branch for the upstream repo (cosmo-celestial) is
master(and notmainor another name) and update the workflow’sref:accordingly if needed.
…me-engine-changes
…me-engine-changes
alepane21
deleted the
ale/eng-7605-force-query-plan-checks-everytime-engine-changes
branch
3 participants
This PR will force the query planner check if the engine has been changed.
Summary by CodeRabbit
Checklist