Update router image and add smoke test workflow#3766
Open
Tharsanan1 wants to merge 4 commits intowso2:cc-mainfrom
Open
Update router image and add smoke test workflow#3766Tharsanan1 wants to merge 4 commits intowso2:cc-mainfrom
Tharsanan1 wants to merge 4 commits intowso2:cc-mainfrom
Conversation
Tharsanan1
requested review from
HeshanSudarshana,
Krishanx92 and
pubudu538
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThese changes introduce router licensing documentation, restructure the Docker build with multi-stage compilation and custom user configuration, add NOTICE license files to the distribution package, implement a comprehensive smoke testing framework via test.sh using Docker Compose orchestration, and establish a GitHub Actions workflow to run automated tests on pull requests. Changes
Sequence Diagram(s)sequenceDiagram
actor TestRunner as Test Runner
participant Compose as Docker Compose
participant Router as Router Service
participant Adapter as Adapter Service
participant Enforcer as Enforcer Service
participant Backend as Mock Backend
TestRunner->>Compose: Start compose stack (docker compose up)
Compose->>Router: Spin up router container
Compose->>Adapter: Spin up adapter container
Compose->>Enforcer: Spin up enforcer container
Compose->>Backend: Spin up mock backend
TestRunner->>Router: Poll /health endpoint
Router-->>TestRunner: Health status
TestRunner->>Router: Poll /ready endpoint
Router-->>TestRunner: Ready status
TestRunner->>Router: POST /testkey (get access token)
Router->>Adapter: Forward request
Adapter->>Enforcer: Validate policies
Enforcer-->>Adapter: Token granted
Adapter-->>Router: Token response
Router-->>TestRunner: Access token
TestRunner->>Router: GET /api with token (Authorization header)
Router->>Adapter: Forward authenticated request
Adapter->>Enforcer: Validate token & policies
Enforcer-->>Adapter: Authorization success
Adapter->>Backend: Forward to backend
Backend-->>Adapter: Response with payload
Adapter-->>Router: Response
Router-->>TestRunner: 200 OK with payload
TestRunner->>Router: GET /api (unauthorized attempt)
Router->>Adapter: Forward unauthenticated request
Adapter->>Enforcer: Validate missing authorization
Enforcer-->>Adapter: Authorization denied
Adapter-->>Router: Rejection
Router-->>TestRunner: Non-200 status code
TestRunner->>Router: Verify LICENSE.txt exists
TestRunner->>Router: Verify NOTICE.txt exists
TestRunner->>Compose: Tear down stack (docker compose down)
Compose-->>TestRunner: Cleanup complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.github/workflows/router-smoke-test.yml (2)
44-55: Consider capturing logs on failure for debugging.The smoke test step looks correct. For easier debugging of failures in CI, consider uploading logs or artifacts when the test fails:
♻️ Suggested artifact upload on failure
- name: Run smoke test env: COMPOSE_PROJECT_NAME: ccsmokegha run: ./test.sh + + - name: Upload logs on failure + if: failure() + uses: actions/upload-artifact@v4 + with: + name: smoke-test-logs + path: | + **/logs/ + **/*.log + retention-days: 7Adjust the
pathpatterns based on where test.sh outputs logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/router-smoke-test.yml around lines 44 - 55, Add a conditional artifact upload when the "Run smoke test" job step fails so CI preserves logs for debugging: after the "Run smoke test" step that runs ./test.sh (with COMPOSE_PROJECT_NAME set), add a subsequent step using actions/upload-artifact (or similar) with if: failure() and path patterns matching where ./test.sh writes logs (e.g., test-logs/**, logs/**, artifacts/**) and a descriptive name like "smoke-test-logs"; ensure the upload step references the same job context and captures any docker/compose logs produced during the run.
16-19: Consider adding concurrency controls.The 120-minute timeout suggests this is a resource-intensive test. Consider adding concurrency controls to cancel in-progress runs when a new commit is pushed to the same PR, saving CI resources:
♻️ Suggested concurrency configuration
jobs: smoke-test: runs-on: ubuntu-22.04 timeout-minutes: 120 + concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/router-smoke-test.yml around lines 16 - 19, The workflow lacks concurrency controls for the long-running smoke-test job ("smoke-test"); add a top-level concurrency stanza to the workflow that defines a unique concurrency group (for example using expressions like github.workflow and github.ref or github.event.pull_request.number) and set cancel-in-progress: true so any in-progress "smoke-test" run is cancelled when a new commit is pushed to the same PR; update the workflow YAML to include this concurrency configuration scope so it applies to the "smoke-test" job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev-scripts/directory-structure-maker/docker-image-builder.sh`:
- Around line 94-96: The script docker-image-builder.sh currently treats
ROUTER/NOTICE.txt as optional but the Dockerfile in router/src/main/resources
unconditionally expects maven/NOTICE.txt; update the if-block that checks
ROUTER/NOTICE.txt so that if the file is missing the script fails fast (print a
clear error mentioning ROUTER/NOTICE.txt and that docker build will fail) and
exits non-zero instead of proceeding to docker build; reference and modify the
existing conditional around ROUTER/NOTICE.txt (the cp to router/maven) to
implement this check-and-exit behavior.
In `@router/src/main/resources/Dockerfile`:
- Around line 41-45: The RUN that creates the system group/user and sets
ownership currently ends with a semicolon before the final echo of MOTD, so
failures in groupadd/useradd/chown can be masked; update the RUN so the MOTD
write is chained with && (or otherwise conditioned) to the preceding commands —
i.e., ensure the groupadd, useradd and chown commands and the bashrc append all
must succeed before executing the echo "${MOTD}" > /etc/motd; locate the RUN
line that contains groupadd, useradd, chown and the MOTD echo in the Dockerfile
and replace the unconditional semicolon separation with a conditional chain so
the step fails fast on any user-setup error.
In `@test.sh`:
- Around line 86-90: The script currently deletes TMP_DIR regardless of
KEEP_RUNNING, which removes the compose file and bind-mounted API artifacts even
when the stack is left running; modify the conditional that checks KEEP_RUNNING
(the same block that calls run_compose down) so that rm -rf "$TMP_DIR" is only
executed when KEEP_RUNNING != "1" (i.e., preserve TMP_DIR when KEEP_RUNNING=1),
ensuring TMP_DIR (and the generated compose file / artifacts) remain available
while the environment is kept up.
- Around line 218-224: The test currently only checks that UNAUTHORIZED_CODE
(set by the curl command) is not "200", which lets other failures (404/503)
pass; change the assertion to require the specific unauthorized status code
(e.g., assert UNAUTHORIZED_CODE == "401" or "403" as your service uses) instead
of "not 200". Update the conditional that uses UNAUTHORIZED_CODE (the curl
assignment and the if block checking "$UNAUTHORIZED_CODE" == "200") to compare
against the expected unauthorized HTTP status and print the error/exit if it
does not match.
- Around line 95-100: The script currently always calls require_command
"$MVN_BIN" making Maven mandatory; update the test.sh flow to check the
SKIP_BUILD variable before invoking require_command for Maven: if SKIP_BUILD is
set/equals 1, skip the require_command "$MVN_BIN" call so prebuilt-image paths
don't require Maven, otherwise keep the existing require_command "$MVN_BIN"
behavior; locate the block with require_command docker, curl, grep, sed and the
require_command "$MVN_BIN" line and wrap or gate the Maven requirement behind a
SKIP_BUILD conditional.
---
Nitpick comments:
In @.github/workflows/router-smoke-test.yml:
- Around line 44-55: Add a conditional artifact upload when the "Run smoke test"
job step fails so CI preserves logs for debugging: after the "Run smoke test"
step that runs ./test.sh (with COMPOSE_PROJECT_NAME set), add a subsequent step
using actions/upload-artifact (or similar) with if: failure() and path patterns
matching where ./test.sh writes logs (e.g., test-logs/**, logs/**, artifacts/**)
and a descriptive name like "smoke-test-logs"; ensure the upload step references
the same job context and captures any docker/compose logs produced during the
run.
- Around line 16-19: The workflow lacks concurrency controls for the
long-running smoke-test job ("smoke-test"); add a top-level concurrency stanza
to the workflow that defines a unique concurrency group (for example using
expressions like github.workflow and github.ref or
github.event.pull_request.number) and set cancel-in-progress: true so any
in-progress "smoke-test" run is cancelled when a new commit is pushed to the
same PR; update the workflow YAML to include this concurrency configuration
scope so it applies to the "smoke-test" job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c854829-b0a2-40c3-b226-2219fd6ec3f9
📒 Files selected for processing (10)
.github/workflows/router-smoke-test.ymldev-scripts/directory-structure-maker/README.mddev-scripts/directory-structure-maker/directory-structure-maker.shdev-scripts/directory-structure-maker/docker-image-builder.shenvoy-filters/mgw-source/filters/http/mgw-wasm-websocket/.bazelversionresources/license/LICENSE-ROUTER.txtresources/license/NOTICE-ROUTER.txtrouter/src/main/assembly/assembly.xmlrouter/src/main/resources/Dockerfiletest.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
$subject
Summary by CodeRabbit