Check the CONTAINER_TOOL value and use it in the e2e test script #29
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hdefazio The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughExports CONTAINER_TOOL in Makefile, enhances check-container-tool to validate CONTAINER_TOOL presence and PATH availability with explicit messages, adds a new check-builder target, and updates test/scripts/run_e2e.sh to parameterize container operations via CONTAINER_TOOL (default docker) for image listing and pulls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Make as make
participant Env as Environment
participant PATH as System PATH
Dev->>Make: make check-container-tool
Make->>Env: Read CONTAINER_TOOL
alt CONTAINER_TOOL unset
Make-->>Dev: Error: CONTAINER_TOOL not set
else CONTAINER_TOOL set
Make->>PATH: which $CONTAINER_TOOL
alt Tool found
Make-->>Dev: OK: container tool available
else Tool missing
Make-->>Dev: Error: tool not in PATH (guidance)
end
end
Dev->>Make: make check-builder
Make->>Env: Read BUILDER
alt BUILDER unset
Make-->>Dev: Message: BUILDER not set
else BUILDER set
Make-->>Dev: Selected builder: $BUILDER
end
sequenceDiagram
autonumber
participant User as Runner
participant E2E as run_e2e.sh
participant Env as Environment
participant CT as $CONTAINER_TOOL (default docker)
note over E2E: Initialize CONTAINER_TOOL (default docker) and print selection
User->>E2E: Execute script
E2E->>Env: Read/Default CONTAINER_TOOL
E2E->>CT: List images to compute SIMTAG
E2E->>CT: Pull inference-sim image
E2E->>CT: Pull inference-scheduler image
E2E->>CT: Pull routing-sidecar image
CT-->>E2E: Success/Failure per operation
E2E-->>User: Continue or exit based on results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
Cannot approve the pull request: Error: openshift-ci[bot] is not included in the approvers role in the OWNERS file |
Signed-off-by: Hannah DeFazio <[email protected]>
Signed-off-by: Hannah DeFazio <[email protected]>
hdefazio
force-pushed
the
check_container_tool
branch
from
5e690c7 to
cb07c97
Compare
![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 (3)
test/scripts/run_e2e.sh (3)
16-18: Consider quoting variables for robustness.The CONTAINER_TOOL variable should be quoted when used in command substitution, and VLLM_SIMULATOR_TAG should be quoted in the grep command to prevent word splitting and globbing issues.
Apply this diff to add proper quoting:
-SIMTAG=$(${CONTAINER_TOOL} images | grep ghcr.io/llm-d/llm-d-inference-sim | awk '{print $2}' | grep ${VLLM_SIMULATOR_TAG}) +SIMTAG=$("${CONTAINER_TOOL}" images | grep ghcr.io/llm-d/llm-d-inference-sim | awk '{print $2}' | grep "${VLLM_SIMULATOR_TAG}") if [[ "${SIMTAG}" != "${VLLM_SIMULATOR_TAG}" ]]; then - ${CONTAINER_TOOL} pull ghcr.io/llm-d/llm-d-inference-sim:${VLLM_SIMULATOR_TAG} + "${CONTAINER_TOOL}" pull ghcr.io/llm-d/llm-d-inference-sim:${VLLM_SIMULATOR_TAG}
25-27: Consider quoting variables for robustness.Similar to the SIMTAG section, CONTAINER_TOOL and EPP_TAG should be quoted to prevent word splitting and globbing issues.
Apply this diff to add proper quoting:
-EPPTAG=$(${CONTAINER_TOOL} images | grep ghcr.io/llm-d/llm-d-inference-scheduler | awk '{print $2}' | grep ${EPP_TAG}) +EPPTAG=$("${CONTAINER_TOOL}" images | grep ghcr.io/llm-d/llm-d-inference-scheduler | awk '{print $2}' | grep "${EPP_TAG}") if [[ "${EPPTAG}" != "${EPP_TAG}" ]]; then - ${CONTAINER_TOOL} pull ghcr.io/llm-d/llm-d-inference-scheduler:${EPP_TAG} + "${CONTAINER_TOOL}" pull ghcr.io/llm-d/llm-d-inference-scheduler:${EPP_TAG}
34-36: Consider quoting variables for robustness.Similar to the previous sections, CONTAINER_TOOL and ROUTING_SIDECAR_TAG should be quoted to prevent word splitting and globbing issues.
Apply this diff to add proper quoting:
-SIDECARTAG=$(${CONTAINER_TOOL} images | grep ghcr.io/llm-d/llm-d-routing-sidecar | awk '{print $2}' | grep ${ROUTING_SIDECAR_TAG}) +SIDECARTAG=$("${CONTAINER_TOOL}" images | grep ghcr.io/llm-d/llm-d-routing-sidecar | awk '{print $2}' | grep "${ROUTING_SIDECAR_TAG}") if [[ "${SIDECARTAG}" != "${ROUTING_SIDECAR_TAG}" ]]; then - ${CONTAINER_TOOL} pull ghcr.io/llm-d/llm-d-routing-sidecar:${ROUTING_SIDECAR_TAG} + "${CONTAINER_TOOL}" pull ghcr.io/llm-d/llm-d-routing-sidecar:${ROUTING_SIDECAR_TAG}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(2 hunks)test/scripts/run_e2e.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
⏰ 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). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (1)
test/scripts/run_e2e.sh (1)
3-5: LGTM!The CONTAINER_TOOL variable declaration with a sensible default and informative echo statement is well-implemented.
|
closed in favor of llm-d#364 |
…lis-patch-1 Update sha and generic fetcher added for zip file download
2 participants
Description
RHOAIENG-35146
How Has This Been Tested?
(this is correct for my system)
Other cases:
Manually set CONTAINER_TOOL := ""
Manually set CONTAINER_TOOL := "docker"
(this is correct for my system)
Merge criteria:
Summary by CodeRabbit
New Features
Improvements
Tests