chore: router ci improvement#2229
Conversation
miklosbarabas
requested review from
JivusAyrus,
StarpTech,
alepane21 and
wilsonrivera
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughHost-level Redis cluster setup step was commented out; Redis ACL creation and health checks now run inside the Redis container via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks✅ Passed checks (3 passed)
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 |
miklosbarabas
force-pushed
the
miklos/chore-router-ci-improvement
branch
from
03d5377 to
740e06b
Compare
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/router-ci.yaml (2)
159-159: Runner label typo will break the job
runs-on: ubuntu-latest-lis invalid; useubuntu-latestto schedule a runner.- runs-on: ubuntu-latest-l + runs-on: ubuntu-latest
282-282: Action input mismatch — workflow passesload_Imagebut the action has no matching input.github/workflows/router-ci.yaml (around line 282) sets
with: load_Image: 'true', but .github/actions/build-push-image/action.yaml declaresinputs:and showsimage_descriptiononly — there is noload_Imageorload_image, so the input will be ignored. Rename the workflow key to the action’s actual input name or add the expected input to action.yaml.
🧹 Nitpick comments (2)
.github/workflows/router-ci.yaml (2)
241-245: Add readiness retry to reduce Redis flakinessCluster may not be ready immediately after the action. Add a simple ping retry before ACL to avoid intermittent failures.
- for port in 7001 7002 7003; do + for port in 7001 7002 7003; do + # wait until node responds + for i in {1..30}; do + if docker exec "$REDIS_CONTAINER" redis-cli -h 127.0.0.1 -p "$port" ping | grep -q PONG; then break; fi + sleep 1 + if [ "$i" -eq 30 ]; then echo "Redis $port not ready"; exit 1; fi + done
242-242: Least privilege for ACL (optional)
+@allgrants everything; if tests don’t require admin commands, restrict to the minimal categories/commands (e.g.,+@read +@write +client +cluster). Otherwise keep as-is for CI convenience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/router-ci.yaml(1 hunks)
⏰ 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: build_test
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/router-ci.yaml (1)
159-159: Runner label typo will break the job
runs-on: ubuntu-latest-lis invalid; should beubuntu-latest.- runs-on: ubuntu-latest-l + runs-on: ubuntu-latest
♻️ Duplicate comments (1)
.github/workflows/router-ci.yaml (1)
241-243: Drop -it, fix quoting for ACL password, and resolve container ID onceInteractive TTY flags can break CI; quoting of
>testis unsafe; and resolving the container twice is brittle. Apply:- for port in 6379; do - docker exec -it $(docker ps --filter "ancestor=redis:7" -q | head -n 1) redis-cli -h 127.0.0.1 -p $port ACL SETUSER cosmo on ">test" "~*" "+@all" - docker exec -it $(docker ps --filter "ancestor=redis:7" -q | head -n 1) redis-cli -u "redis://cosmo:test@127.0.0.1:$port" ping + REDIS_CONTAINER="$(docker ps --filter 'ancestor=redis:7' --format '{{.ID}}' | head -n1)" + if [ -z "$REDIS_CONTAINER" ]; then echo 'redis:7 container not found'; exit 1; fi + for port in 6379; do + docker exec "$REDIS_CONTAINER" redis-cli -h 127.0.0.1 -p "$port" ACL SETUSER cosmo on '>test' '~*' '+@all' + docker exec "$REDIS_CONTAINER" redis-cli --user cosmo --pass test -h 127.0.0.1 -p "$port" ping done
🧹 Nitpick comments (3)
.github/workflows/router-ci.yaml (3)
241-245: Single‑port loop is unnecessaryIf we only ever touch 6379, simplify for readability.
- for port in 6379; do - docker exec "$REDIS_CONTAINER" redis-cli -h 127.0.0.1 -p "$port" ACL SETUSER cosmo on '>test' '~*' '+@all' - docker exec "$REDIS_CONTAINER" redis-cli --user cosmo --pass test -h 127.0.0.1 -p "$port" ping - echo "ACL user 'cosmo' created with full access on port $port" - done + port=6379 + docker exec "$REDIS_CONTAINER" redis-cli -h 127.0.0.1 -p "$port" ACL SETUSER cosmo on '>test' '~*' '+@all' + docker exec "$REDIS_CONTAINER" redis-cli --user cosmo --pass test -h 127.0.0.1 -p "$port" ping + echo "ACL user 'cosmo' created with full access on port $port"
227-236: Disabling Redis cluster: confirm no tests depend on itYou commented out cluster setup. Ensure cluster‑dependent tests are gated/filtered, or provide a lightweight replacement.
If needed, I can add a conditional that only enables cluster when the test target matches cluster suites.
239-239: Reduce noisy logs
docker ps -aadds noise and can leak unrelated container info. Drop unless debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/router-ci.yaml(1 hunks)
⏰ 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). (10)
- GitHub Check: build-router
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
| slave2-port: 7005 | ||
| slave3-port: 7006 | ||
| sleep-duration: 5 | ||
| # - name: Setup Redis Cluster (for Cluster tests) |
There was a problem hiding this comment.
Will clean everything up i touch once im done 👍
- renamed TestNatsEvents to TestFlakyNatsEvents to be retried on failure - commented out suspected faulty test
- cleanup
…nt' into miklos/chore-router-ci-improvement # Conflicts: # .github/workflows/router-ci.yaml
|
closing in favor of #2234 |
5 participants
Summary by CodeRabbit
Checklist