Conversation
…with-existing-cluster' Signed-off-by: Jooho Lee <jlee@redhat.com>
|
""" WalkthroughThis update introduces new configuration files and scripts to support KServe end-to-end testing, enhances MinIO and operator deployment flexibility with additional environment variables and improved SSL handling, and refines test execution and shell command handling logic. It also adds targeted functional tests and a new playbook for orchestrating KServe e2e tests on existing clusters. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainScript as main.sh (KServe E2E)
participant Cluster
participant MinIO
participant Pytest
User->>MainScript: Set environment variables and run script
MainScript->>Cluster: Login (if not KIND)
MainScript->>MinIO: Ensure MinIO deployed and configured
MainScript->>Cluster: Prepare namespaces and secrets
MainScript->>Pytest: Execute e2e tests with uv run pytest
Pytest-->>MainScript: Return test results
MainScript->>User: Report test outcome
sequenceDiagram
participant User
participant DeployScript as main.sh (MinIO Deploy)
participant Cluster
User->>DeployScript: Set SSL and cluster env vars, run script
DeployScript->>DeployScript: Validate SSL flags
DeployScript->>Cluster: Apply manifests
alt enable_ssl
DeployScript->>Cluster: Create re-encrypt route
else edge_enable_ssl
DeployScript->>Cluster: Create edge route
else
DeployScript->>Cluster: Expose service
end
DeployScript->>Cluster: Wait for pods ready
DeployScript->>User: Output MinIO service URL
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)`**/*`: Use lowercase with underscores for directories and files (e.g., routers/user_routes.py).
`tests/**/*`: Place all test code in a separate 'tests' directory.
🪛 Ruff (0.11.9)tests/fvt/roles/non-cluster-tests/shell_execute/test_fvt_shell_execute.py295-295: f-string without any placeholders Remove extraneous (F541) 296-296: f-string without any placeholders Remove extraneous (F541) 🪛 Flake8 (7.2.0)tests/fvt/roles/non-cluster-tests/shell_execute/test_fvt_shell_execute.py[error] 295-295: f-string is missing placeholders (F541) [error] 296-296: f-string is missing placeholders (F541) ⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
src/roles/minio/deploy/main.sh (1)
50-58: Correct use ofis_positivefor SSL flags
Assigningenable_ssl=$(is_positive ${ENABLE_SSL})captures stdout, not the function’s exit code. Instead, test its exit status directly:if is_positive "${ENABLE_SSL}"; then info "MINIO SSL Enabled" … fior explicitly capture
$?intoenable_sslafter calling the function.
🧹 Nitpick comments (17)
src/roles/shell/execute/main.py (1)
139-140: Use more pythonic syntax for membership test.The filtering logic is correct, but consider using the more pythonic
not insyntax for the membership test.- if not "Cloning" in line: + if "Cloning" not in line:src/commons/scripts/utils.sh (1)
535-563: New utility function looks good with minor improvement needed.The
wait_for_zero_countfunction is well-implemented and follows established patterns in the file. However, address the static analysis warning about masking return values.Apply this diff to fix the static analysis warning:
- local count=$(eval "$command") + local count + count=$(eval "$command")src/roles/minio/deploy/config.yaml (1)
50-52: Fix trailing spaces in YAML.The
CLUSTER_TYPEvariable is a useful addition for cluster-specific logic.Apply this diff to fix the trailing spaces:
- name: CLUSTER_TYPE description: cluster type - default: "OCP" + default: "OCP"unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml (1)
1-115: Excellent playbook design with comprehensive documentation.The playbook provides a well-structured approach to KServe e2e testing with excellent examples for both PR and local build workflows. However, please fix the trailing spaces throughout the file.
Apply this diff to fix trailing spaces:
description: | This playbook configure test cluster that install all required objects for e2e tests and run test. - By default, it will use the fast channel of kserve/odh-model-controller. If you want to test with custom build, you can set the following cases: - + By default, it will use the fast channel of kserve/odh-model-controller. If you want to test with custom build, you can set the following cases: + Environment variables: - USER_NAME - PR_USER_NAME - REPO_NAME - PR_REPO_URL - PR_BRANCH_NAME - REPO_URL - KO_DOCKER_REPO - TAG - + Example: * PR case commands: ~~~ @@ -29,10 +29,10 @@ export KO_DOCKER_REPO=quay.io/${USER_NAME} export TAG=loopy git clone --branch $PR_BRANCH_NAME $REPO_URL - cd $REPO_NAME + cd $REPO_NAME git remote add ${USER_NAME} ${REPO_URL} - + make docker-build docker tag docker.io/library/kserve-controller:latest ${KO_DOCKER_REPO}/kserve-controller:${TAG} docker push ${KO_DOCKER_REPO}/kserve-controller:${TAG} @@ -46,8 +46,8 @@ export CUSTOM_KSERVE_MANIFESTS=https://github.com/${USER_NAME}/kserve/tarball/e2e-test-devflag ./loopy playbook run kserve-e2e-test-with-existing-cluster ~~~ - - * Local build: + + * Local build: ~~~ export USER_NAME=jooholee export REPO_NAME=kserve @@ -75,7 +75,7 @@ name: install-odh-stable-operator description: Install ODH operator fast channel input_env: - CHANNEL: "fast" + CHANNEL: "fast" - unit: name: install-serverless-stable-operator - unit: @@ -91,7 +91,7 @@ - role: name: opendatahub-create-dsc - description: Create a DSC + description: Create a DSC input_env: OPENDATAHUB_TYPE: opendatahub ENABLE_KSERVE: Managedsrc/roles/kserve-tests/e2e-test/main.sh (8)
23-23: Quote sourced path to avoid word splitting
Usesource "$root_directory/src/commons/scripts/utils.sh"to guard against spaces or special characters in the path.
27-29: Remove unused variablerole_name
role_nameis assigned but never used. Either leverage it (e.g., for logging) or remove the assignment to clean up the script.
30-30: Handlecdfailure explicitly
Usecd "$ROLE_DIR" || exit 1to abort if the directory change fails (apply the same pattern to subsequentcdcommands).
32-35: Add error handling for repository checkout
Aftergit clone, addcd opendatahub-tests || exit 1so the script fails fast if the clone or directory change doesn’t succeed.
37-37: Remove or utilize theresultvariable
resultis initialized to1but never used before the finalecho "$?". Consider removing this assignment or consistently usingresultto capture and propagate the script’s status.
70-73: Guard against missingfirst_user_indexin sed replacement
Ifgrepreturns no match,line_numberwill be empty andsed -iwill fail. Add a check:if [[ -z "$line_number" ]]; then echo "Error: first_user_index not found in tests/conftest.py" >&2 exit 1 fibefore invoking
sed.
77-81: Quote markers and paths in pytest invocation
Use-m "${PYTEST_MARKER}"and"${PYTEST_PATH}"to avoid word splitting if these variables contain spaces or special characters.
89-91: Quote variables when writing to report file
Change:echo "${index_role_name}::$?" >>${REPORT_FILE}to:
echo "${index_role_name}::$?" >> "${REPORT_FILE}"to avoid issues if
REPORT_FILEcontains spaces.src/roles/minio/deploy/main.sh (5)
44-49: Quote paths insedoutput redirection
The redirection uses unquoted expansions:> ${ROLE_DIR}/$(basename $minio_deployment_manifests_path)Wrap variables in quotes:
> "${ROLE_DIR}/$(basename "${minio_deployment_manifests_path}")"
65-69: Clean up commented-out error handling
Lines 67–68 leaveresult=1andstop_when_error_happenedcommented. Remove these stubs or restore the intended logic to avoid confusion.
72-72: Quote variables inoc applycommand
Change:oc apply -f ${ROLE_DIR}/$(basename $minio_deployment_manifests_path)to:
oc apply -f "${ROLE_DIR}/$(basename "${minio_deployment_manifests_path}")"
118-124: Quotejsonpathargument and variables when retrieving route host
Wrap the jsonpath string and variable expansions:minio_svc_url=$(oc get route minio -o jsonpath='{.spec.host}')and use:
echo "MINIO_S3_SVC_URL=${minio_svc_protocol}://${minio_svc_url}"within quotes.
129-133: Quote environment file writes
Ensure eachechoto${OUTPUT_ENV_FILE}is redirecting into a quoted path:echo "MINIO_DEFAULT_BUCKET_NAME=${DEFAULT_BUCKET_NAME}" >> "${OUTPUT_ENV_FILE}"This prevents issues if
OUTPUT_ENV_FILEcontains spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
requirements.txt(1 hunks)src/commons/scripts/utils.sh(2 hunks)src/roles/kserve-tests/e2e-test/config.yaml(1 hunks)src/roles/kserve-tests/e2e-test/main.sh(1 hunks)src/roles/minio/deploy/config.yaml(2 hunks)src/roles/minio/deploy/main.sh(2 hunks)src/roles/nfs-provisioner/main.sh(1 hunks)src/roles/operator/install/config.yaml(1 hunks)src/roles/operator/install/main.sh(1 hunks)src/roles/shell/execute/main.py(1 hunks)tests/fvt/roles/non-cluster-tests/shell_execute/test_fvt_shell_execute.py(1 hunks)unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
src/roles/shell/execute/main.py
139-139: Test for membership should be not in
Convert to not in
(E713)
tests/fvt/roles/non-cluster-tests/shell_execute/test_fvt_shell_execute.py
295-295: f-string without any placeholders
Remove extraneous f prefix
(F541)
296-296: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 YAMLlint (1.37.1)
src/roles/minio/deploy/config.yaml
[error] 52-52: trailing spaces
(trailing-spaces)
src/roles/kserve-tests/e2e-test/config.yaml
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 69-69: no new line character at the end of file
(new-line-at-end-of-file)
[error] 69-69: trailing spaces
(trailing-spaces)
unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 94-94: trailing spaces
(trailing-spaces)
🪛 Shellcheck (0.10.0)
src/commons/scripts/utils.sh
[warning] 548-548: Declare and assign separately to avoid masking return values.
(SC2155)
src/roles/kserve-tests/e2e-test/main.sh
[warning] 28-28: role_name appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 30-30: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 37-37: result appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 43-43: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 46-46: You are missing a required space here.
(SC1035)
src/roles/minio/deploy/main.sh
[warning] 72-72: Quote this to prevent word splitting.
(SC2046)
[warning] 75-75: edge_enable_ssl is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cluster-test (3.12)
🔇 Additional comments (7)
src/roles/nfs-provisioner/main.sh (1)
35-35: Good cleanup of leftover debug code.Removing the debug line "JHOUSE --- TEST" is appropriate as it appears to be leftover test/debug code that shouldn't be in production.
requirements.txt (1)
12-13: LGTM! Dependencies added for new testing framework.The addition of
uv(fast Python package installer) andpytest_jira(Jira integration for pytest) aligns well with the new KServe e2e testing functionality being introduced in this PR.src/roles/operator/install/config.yaml (1)
80-83: Good addition of configurable operator group name.The new
OPERATORGROUP_NAMEenvironment variable provides useful flexibility for operator group naming while maintaining a sensible default value. The description clearly explains the behavior.src/roles/operator/install/main.sh (1)
56-56: Properly aligned with the new OPERATORGROUP_NAME configuration.The change from
$SUBSCRIPTION_NAMEto$OPERATORGROUP_NAMEcorrectly implements the configurable operator group naming introduced in the config.yaml file.src/commons/scripts/utils.sh (1)
193-193: LGTM! Timeout calculation fix is correct.The formula correctly calculates elapsed time in minutes:
wait_counter * 10 seconds / 60 = minutes.src/roles/minio/deploy/config.yaml (1)
38-40: Good addition for SSL configuration flexibility.The
EDGE_ENABLE_SSLvariable provides a clean way to enable SSL via router, distinct from the existingENABLE_SSLflag.src/roles/kserve-tests/e2e-test/main.sh (1)
3-5: ```shell
#!/bin/bash
set -eecho "Searching for all references to DEBUG in the repository..."
rg -n '\bDEBUG\b' .echo
echo "Searching for assignments to DEBUG..."
rg -n 'DEBUG=' .echo
echo "Displaying the first 50 lines of src/roles/kserve-tests/e2e-test/main.sh for context..."
sed -n '1,50p' src/roles/kserve-tests/e2e-test/main.sh</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
|
||
| @pytest.mark.fvt | ||
| @pytest.mark.fvt_roles | ||
| def test_shell_execute_git_clone(role_dir, base_env, setup_test_env): | ||
| """Test executing git clone command and verify the directory is created""" | ||
| test_env = { | ||
| "COMMANDS": "cd /tmp \n git clone git@github.com:jooho/loopy.git", | ||
| "SHOW_COMMAND": "true", | ||
| } | ||
|
|
||
| env = setup_test_env(test_env) | ||
|
|
||
| # Run the role | ||
| result = subprocess.run( | ||
| ["python3", str(role_dir / "main.py")], env=env, capture_output=True, text=True | ||
| ) | ||
|
|
||
| assert result.returncode == 0, f"Role execution failed: {result.stderr}" | ||
|
|
||
| # Verify command output file exists | ||
| output_file = Path(base_env["ROLE_DIR"]) / "0-command.txt" | ||
| assert output_file.exists() | ||
|
|
||
| # Verify that /tmp/loopy directory was created | ||
| loopy_dir = Path("/tmp/loopy") | ||
| assert loopy_dir.exists(), f"Expected directory /tmp/loopy was not created" | ||
| assert loopy_dir.is_dir(), f"/tmp/loopy exists but is not a directory" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test robustness and fix formatting issues.
The test has several issues that should be addressed:
- SSH git clone may fail in CI environments without SSH keys configured
- The test doesn't clean up the
/tmp/loopydirectory it creates - Unnecessary f-string prefixes on assertion messages
Consider these improvements:
+import shutil
def test_shell_execute_git_clone(role_dir, base_env, setup_test_env):
"""Test executing git clone command and verify the directory is created"""
+ # Use HTTPS instead of SSH to avoid authentication issues in CI
test_env = {
- "COMMANDS": "cd /tmp \n git clone git@github.com:jooho/loopy.git",
+ "COMMANDS": "cd /tmp \n git clone https://github.com/jooho/loopy.git",
"SHOW_COMMAND": "true",
}
env = setup_test_env(test_env)
+
+ # Ensure clean state
+ loopy_dir = Path("/tmp/loopy")
+ if loopy_dir.exists():
+ shutil.rmtree(loopy_dir)
# Run the role
result = subprocess.run(
["python3", str(role_dir / "main.py")], env=env, capture_output=True, text=True
)
assert result.returncode == 0, f"Role execution failed: {result.stderr}"
# Verify command output file exists
output_file = Path(base_env["ROLE_DIR"]) / "0-command.txt"
assert output_file.exists()
# Verify that /tmp/loopy directory was created
- loopy_dir = Path("/tmp/loopy")
- assert loopy_dir.exists(), f"Expected directory /tmp/loopy was not created"
- assert loopy_dir.is_dir(), f"/tmp/loopy exists but is not a directory"
+ assert loopy_dir.exists(), "Expected directory /tmp/loopy was not created"
+ assert loopy_dir.is_dir(), "/tmp/loopy exists but is not a directory"
+
+ # Cleanup
+ shutil.rmtree(loopy_dir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.fvt | |
| @pytest.mark.fvt_roles | |
| def test_shell_execute_git_clone(role_dir, base_env, setup_test_env): | |
| """Test executing git clone command and verify the directory is created""" | |
| test_env = { | |
| "COMMANDS": "cd /tmp \n git clone git@github.com:jooho/loopy.git", | |
| "SHOW_COMMAND": "true", | |
| } | |
| env = setup_test_env(test_env) | |
| # Run the role | |
| result = subprocess.run( | |
| ["python3", str(role_dir / "main.py")], env=env, capture_output=True, text=True | |
| ) | |
| assert result.returncode == 0, f"Role execution failed: {result.stderr}" | |
| # Verify command output file exists | |
| output_file = Path(base_env["ROLE_DIR"]) / "0-command.txt" | |
| assert output_file.exists() | |
| # Verify that /tmp/loopy directory was created | |
| loopy_dir = Path("/tmp/loopy") | |
| assert loopy_dir.exists(), f"Expected directory /tmp/loopy was not created" | |
| assert loopy_dir.is_dir(), f"/tmp/loopy exists but is not a directory" | |
| import shutil | |
| @pytest.mark.fvt | |
| @pytest.mark.fvt_roles | |
| def test_shell_execute_git_clone(role_dir, base_env, setup_test_env): | |
| """Test executing git clone command and verify the directory is created""" | |
| # Use HTTPS instead of SSH to avoid authentication issues in CI | |
| test_env = { | |
| "COMMANDS": "cd /tmp \n git clone https://github.com/jooho/loopy.git", | |
| "SHOW_COMMAND": "true", | |
| } | |
| env = setup_test_env(test_env) | |
| # Ensure clean state | |
| loopy_dir = Path("/tmp/loopy") | |
| if loopy_dir.exists(): | |
| shutil.rmtree(loopy_dir) | |
| # Run the role | |
| result = subprocess.run( | |
| ["python3", str(role_dir / "main.py")], env=env, capture_output=True, text=True | |
| ) | |
| assert result.returncode == 0, f"Role execution failed: {result.stderr}" | |
| # Verify command output file exists | |
| output_file = Path(base_env["ROLE_DIR"]) / "0-command.txt" | |
| assert output_file.exists() | |
| # Verify that /tmp/loopy directory was created | |
| assert loopy_dir.exists(), "Expected directory /tmp/loopy was not created" | |
| assert loopy_dir.is_dir(), "/tmp/loopy exists but is not a directory" | |
| # Cleanup | |
| shutil.rmtree(loopy_dir) |
🧰 Tools
🪛 Ruff (0.11.9)
295-295: f-string without any placeholders
Remove extraneous f prefix
(F541)
296-296: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In tests/fvt/roles/non-cluster-tests/shell_execute/test_fvt_shell_execute.py
around lines 270 to 296, the test_shell_execute_git_clone function has issues
with SSH git clone possibly failing in CI due to missing SSH keys, it does not
clean up the /tmp/loopy directory after the test, and uses unnecessary f-string
prefixes in assertion messages. To fix this, modify the git clone command to use
HTTPS instead of SSH to avoid SSH key dependency, add code to remove the
/tmp/loopy directory after the test completes to prevent leftover state, and
remove the f-string prefixes from assertion messages where no variable
interpolation is needed.
| role: | ||
| created_date: "20250620" | ||
| name: kserve-odh-e2e-test | ||
| description: | | ||
| This role help to run kserve e2e test. | ||
| The default execution run modelserving test cases. | ||
|
|
||
| pre-requirements: | ||
| - openshift cluster | ||
|
|
||
| Input Environment: | ||
| The parameters include: | ||
| - CLUSTER_TYPE: cluster type | ||
| - CLUSTER_NAMESPACE: cluster namespace | ||
| - CLUSTER_API_URL: openshift api url | ||
| - CLUSTER_ADMIN_ID: openshift cluster user id | ||
| - CLUSTER_ADMIN_PW: openshift cluster user password | ||
| - PYTEST_JIRA_TOKEN: pytest jira token | ||
| - MINIO_S3_SVC_URL: minio s3 service url | ||
| - MINIO_ACCESS_KEY_ID: minio access key id | ||
| - MINIO_SECRET_ACCESS_KEY: minio secret access key | ||
| - PYTEST_PATH: pytest path | ||
| - PYTEST_MARKER: Set pytest marker, if you want to run specific test cases. | ||
|
|
||
| To run it: | ||
| ./loopy roles run kserve-odh-e2e-test \ | ||
| -p CLUSTER_TYPE=OCP \ | ||
| -p CLUSTER_API_URL=https://api.cluster-name.example.com:6443 \ | ||
| -p CLUSTER_ADMIN_ID=admin \ | ||
| -p CLUSTER_ADMIN_PW=password \ | ||
| -p PYTEST_JIRA_TOKEN=pytest-jira-token \ | ||
| -p MINIO_S3_SVC_URL=https://minio.minio.svc.cluster.local:9000 \ | ||
| -p MINIO_ACCESS_KEY_ID=admin \ | ||
| -p MINIO_SECRET_ACCESS_KEY=password \ | ||
| -p PYTEST_PATH=tests/model_serving | ||
|
|
||
| input_env: | ||
| - name: CLUSTER_TYPE | ||
| description: cluster type | ||
| default: "OCP" | ||
|
|
||
| - name: CLUSTER_API_URL | ||
| description: openshift api url | ||
| required: true | ||
| - name: CLUSTER_ADMIN_ID | ||
| description: openshift cluster user id | ||
| required: true | ||
| - name: CLUSTER_ADMIN_PW | ||
| description: openshift cluster user password | ||
| required: true | ||
| - name: PYTEST_JIRA_TOKEN | ||
| description: pytest jira token | ||
| required: true | ||
| - name: MINIO_S3_SVC_URL | ||
| description: Set minio s3 service url,if you want to use your own minio. (HTTPS is required) | ||
| - name: MINIO_ACCESS_KEY_ID | ||
| description: minio access key id | ||
| default: "admin" | ||
| - name: MINIO_SECRET_ACCESS_KEY | ||
| description: minio secret access key | ||
| default: "password" | ||
| - name: PYTEST_PATH | ||
| description: Set other path, if you want to run specific test cases | ||
| default: "tests/model_serving" | ||
| - name: PYTEST_MARKER | ||
| description: Set pytest marker, if you want to run specific test cases. | ||
| default: "" | ||
| - name: CUSTOM_KSERVE_IMG | ||
|
No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix YAML formatting issues and complete the configuration.
The KServe e2e test role configuration is well-structured but has several formatting issues and incomplete content.
Apply these fixes:
description: |
- This role help to run kserve e2e test.
+ This role help to run kserve e2e test.
The default execution run modelserving test cases.
pre-requirements:
@@ -32,9 +32,9 @@
-p MINIO_S3_SVC_URL=https://minio.minio.svc.cluster.local:9000 \
-p MINIO_ACCESS_KEY_ID=admin \
-p MINIO_SECRET_ACCESS_KEY=password \
- -p PYTEST_PATH=tests/model_serving
+ -p PYTEST_PATH=tests/model_serving
input_env:
- name: CLUSTER_TYPE
description: cluster type
- default: "OCP"
-
+ default: "OCP"
+
- name: CLUSTER_API_URL
description: openshift api url
required: true
@@ -66,4 +66,6 @@
description: Set pytest marker, if you want to run specific test cases.
default: ""
- name: CUSTOM_KSERVE_IMG
-
+ description: Custom KServe image for testing
+ default: ""
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| role: | |
| created_date: "20250620" | |
| name: kserve-odh-e2e-test | |
| description: | | |
| This role help to run kserve e2e test. | |
| The default execution run modelserving test cases. | |
| pre-requirements: | |
| - openshift cluster | |
| Input Environment: | |
| The parameters include: | |
| - CLUSTER_TYPE: cluster type | |
| - CLUSTER_NAMESPACE: cluster namespace | |
| - CLUSTER_API_URL: openshift api url | |
| - CLUSTER_ADMIN_ID: openshift cluster user id | |
| - CLUSTER_ADMIN_PW: openshift cluster user password | |
| - PYTEST_JIRA_TOKEN: pytest jira token | |
| - MINIO_S3_SVC_URL: minio s3 service url | |
| - MINIO_ACCESS_KEY_ID: minio access key id | |
| - MINIO_SECRET_ACCESS_KEY: minio secret access key | |
| - PYTEST_PATH: pytest path | |
| - PYTEST_MARKER: Set pytest marker, if you want to run specific test cases. | |
| To run it: | |
| ./loopy roles run kserve-odh-e2e-test \ | |
| -p CLUSTER_TYPE=OCP \ | |
| -p CLUSTER_API_URL=https://api.cluster-name.example.com:6443 \ | |
| -p CLUSTER_ADMIN_ID=admin \ | |
| -p CLUSTER_ADMIN_PW=password \ | |
| -p PYTEST_JIRA_TOKEN=pytest-jira-token \ | |
| -p MINIO_S3_SVC_URL=https://minio.minio.svc.cluster.local:9000 \ | |
| -p MINIO_ACCESS_KEY_ID=admin \ | |
| -p MINIO_SECRET_ACCESS_KEY=password \ | |
| -p PYTEST_PATH=tests/model_serving | |
| input_env: | |
| - name: CLUSTER_TYPE | |
| description: cluster type | |
| default: "OCP" | |
| - name: CLUSTER_API_URL | |
| description: openshift api url | |
| required: true | |
| - name: CLUSTER_ADMIN_ID | |
| description: openshift cluster user id | |
| required: true | |
| - name: CLUSTER_ADMIN_PW | |
| description: openshift cluster user password | |
| required: true | |
| - name: PYTEST_JIRA_TOKEN | |
| description: pytest jira token | |
| required: true | |
| - name: MINIO_S3_SVC_URL | |
| description: Set minio s3 service url,if you want to use your own minio. (HTTPS is required) | |
| - name: MINIO_ACCESS_KEY_ID | |
| description: minio access key id | |
| default: "admin" | |
| - name: MINIO_SECRET_ACCESS_KEY | |
| description: minio secret access key | |
| default: "password" | |
| - name: PYTEST_PATH | |
| description: Set other path, if you want to run specific test cases | |
| default: "tests/model_serving" | |
| - name: PYTEST_MARKER | |
| description: Set pytest marker, if you want to run specific test cases. | |
| default: "" | |
| - name: CUSTOM_KSERVE_IMG | |
| role: | |
| created_date: "20250620" | |
| name: kserve-odh-e2e-test | |
| description: | | |
| This role help to run kserve e2e test. | |
| The default execution run modelserving test cases. | |
| pre-requirements: | |
| - openshift cluster | |
| Input Environment: | |
| The parameters include: | |
| - CLUSTER_TYPE: cluster type | |
| - CLUSTER_NAMESPACE: cluster namespace | |
| - CLUSTER_API_URL: openshift api url | |
| - CLUSTER_ADMIN_ID: openshift cluster user id | |
| - CLUSTER_ADMIN_PW: openshift cluster user password | |
| - PYTEST_JIRA_TOKEN: pytest jira token | |
| - MINIO_S3_SVC_URL: minio s3 service url | |
| - MINIO_ACCESS_KEY_ID: minio access key id | |
| - MINIO_SECRET_ACCESS_KEY: minio secret access key | |
| - PYTEST_PATH: pytest path | |
| - PYTEST_MARKER: Set pytest marker, if you want to run specific test cases. | |
| To run it: | |
| ./loopy roles run kserve-odh-e2e-test \ | |
| -p CLUSTER_TYPE=OCP \ | |
| -p CLUSTER_API_URL=https://api.cluster-name.example.com:6443 \ | |
| -p CLUSTER_ADMIN_ID=admin \ | |
| -p CLUSTER_ADMIN_PW=password \ | |
| -p PYTEST_JIRA_TOKEN=pytest-jira-token \ | |
| -p MINIO_S3_SVC_URL=https://minio.minio.svc.cluster.local:9000 \ | |
| -p MINIO_ACCESS_KEY_ID=admin \ | |
| -p MINIO_SECRET_ACCESS_KEY=password \ | |
| -p PYTEST_PATH=tests/model_serving | |
| input_env: | |
| - name: CLUSTER_TYPE | |
| description: cluster type | |
| default: "OCP" | |
| - name: CLUSTER_API_URL | |
| description: openshift api url | |
| required: true | |
| - name: CLUSTER_ADMIN_ID | |
| description: openshift cluster user id | |
| required: true | |
| - name: CLUSTER_ADMIN_PW | |
| description: openshift cluster user password | |
| required: true | |
| - name: PYTEST_JIRA_TOKEN | |
| description: pytest jira token | |
| required: true | |
| - name: MINIO_S3_SVC_URL | |
| description: Set minio s3 service url,if you want to use your own minio. (HTTPS is required) | |
| - name: MINIO_ACCESS_KEY_ID | |
| description: minio access key id | |
| default: "admin" | |
| - name: MINIO_SECRET_ACCESS_KEY | |
| description: minio secret access key | |
| default: "password" | |
| - name: PYTEST_PATH | |
| description: Set other path, if you want to run specific test cases | |
| default: "tests/model_serving" | |
| - name: PYTEST_MARKER | |
| description: Set pytest marker, if you want to run specific test cases. | |
| default: "" | |
| - name: CUSTOM_KSERVE_IMG | |
| description: Custom KServe image for testing | |
| default: "" |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 69-69: no new line character at the end of file
(new-line-at-end-of-file)
[error] 69-69: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In src/roles/kserve-tests/e2e-test/config.yaml lines 1 to 69, fix YAML
formatting issues by ensuring consistent indentation, proper spacing after
colons, and correct list item formatting. Complete the configuration by
providing a description and default or required status for the CUSTOM_KSERVE_IMG
input_env entry. Review the entire file for any trailing incomplete lines or
missing values and correct them to maintain valid YAML syntax.
| github_dir="$current_dir" | ||
| while [ ! -d "$github_dir/.git" ] && [ "$github_dir" != "/" ]; do | ||
| github_dir="$(dirname "$github_dir")" | ||
| done | ||
|
|
||
| # If the .github folder is found, set root_directory | ||
| if [ -d "$github_dir/.git" ]; then | ||
| root_directory="$github_dir" | ||
| echo "The root directory is: $root_directory" | ||
| else | ||
| echo "Error: Unable to find .github folder." | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exit on failure to locate repository root
The script echoes an error when it can’t find the .git directory but continues execution. Replace the echo on line 21 with an error to stderr and an exit, e.g.:
echo "Error: .git directory not found" >&2
exit 1This prevents subsequent commands from running in an undefined context.
🤖 Prompt for AI Agents
In src/roles/kserve-tests/e2e-test/main.sh around lines 11 to 22, the script
currently echoes an error message to stdout when it fails to find the .git
directory but continues execution. Replace the echo statement on line 21 with an
error message sent to stderr using >&2 and immediately follow it with exit 1 to
stop the script, preventing further commands from running in an undefined
context.
| if [[ $(which oc) >/dev/null ]]; then | ||
| oc_path=$(which oc) | ||
| else | ||
| oc_path=$root_directory/bin/oc | ||
| fi |
There was a problem hiding this comment.
Fix which oc conditional and quoting
The test if [[ $(which oc) >/dev/null ]]; then is invalid and doesn’t inspect exit status. Replace with:
if which oc >/dev/null 2>&1; then
oc_path="$(which oc)"
else
oc_path="$root_directory/bin/oc"
fiAlso wrap expansions in quotes to prevent word splitting.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 46-46: You are missing a required space here.
(SC1035)
🤖 Prompt for AI Agents
In src/roles/kserve-tests/e2e-test/main.sh around lines 46 to 50, the
conditional test using `if [[ $(which oc) >/dev/null ]]; then` is incorrect
because it checks command output instead of exit status. Replace it with `if
which oc >/dev/null 2>&1; then` to properly test if `oc` is available. Also,
wrap variable expansions like `$(which oc)` and `$root_directory/bin/oc` in
double quotes to prevent word splitting and ensure correct path handling.
| can_create_route=0 | ||
| if [[ ${edge_enable_ssl} == "0" && ${enable_ssl} == "0" ]]; then | ||
| fail "EDGE_ENABLE_SSL and ENABLE_SSL cannot be set at the same time" | ||
| can_create_route=1 | ||
| result=1 | ||
| stop_when_error_happened $result $index_role_name $REPORT_FILE true | ||
| fi |
There was a problem hiding this comment.
Initialize edge_enable_ssl before use and fix conflict logic
edge_enable_ssl is referenced on line 75 but never set (SC2154). Mirror the enable_ssl pattern:
edge_enable_ssl=$(is_positive "${EDGE_ENABLE_SSL}")Then validate both flags correctly before creating routes.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 75-75: edge_enable_ssl is referenced but not assigned.
(SC2154)
🤖 Prompt for AI Agents
In src/roles/minio/deploy/main.sh around lines 74 to 80, initialize the variable
edge_enable_ssl before using it by assigning it the result of is_positive with
EDGE_ENABLE_SSL as input, similar to how enable_ssl is set. Then adjust the
conditional logic to correctly validate that both edge_enable_ssl and enable_ssl
are not enabled simultaneously before proceeding, ensuring the conflict check is
accurate and the script behaves as intended.
Signed-off-by: Jooho Lee <jlee@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/roles/minio/deploy/main.sh (1)
74-80: Initializeedge_enable_sslbefore conflict check
edge_enable_sslis referenced here but never set (SC2154), leading to an unbound variable. Mirror the existing pattern forenable_ssl:+ edge_enable_ssl=$(is_positive "${EDGE_ENABLE_SSL}") can_create_route=0 if [[ ${edge_enable_ssl} == "0" && ${enable_ssl} == "0" ]]; then …src/roles/kserve-tests/e2e-test/main.sh (2)
19-22: Exit on failure finding repository root
Currently errors finding.gitbut continues. Replace the echo with an error to stderr and exit to avoid undefined behavior:echo "Error: Unable to find .git directory" >&2 exit 1
46-50: Correctwhich occonditional and add quoting
The checkif [[ $(which oc) >/dev/null ]];inspects output, not exit status. Update to:if which oc >/dev/null 2>&1; then oc_path="$(which oc)" else oc_path="$root_directory/bin/oc" fiAlso quote expansions (
"$root_directory").
🧹 Nitpick comments (6)
src/roles/minio/deploy/main.sh (3)
82-94: Quote variables in route creation commands
When creating routes or exposing services, wrap paths and namespaces in quotes to prevent word splitting:oc create route reencrypt minio \ --service="minio" --port="minio-client-port" \ --dest-ca-cert="${ROLE_DIR}/root.crt" -n "${MINIO_NAMESPACE}"Applying this across the block improves robustness.
98-102: Parameterize pod-ready timeout
You now pass100whenMINIO_IMAGEis set. Extract this into a variable (POD_READINESS_TIMEOUT) for clarity and easier tuning.
118-126: Quote output file redirection target and values
Ensure the output file path and URL components are quoted:- echo "MINIO_S3_SVC_URL=${minio_svc_protocol}://${minio_svc_url}" >>${OUTPUT_ENV_FILE} + echo "MINIO_S3_SVC_URL=${minio_svc_protocol}://${minio_svc_url}" >> "${OUTPUT_ENV_FILE}"Apply similar quoting to the other
echolines.unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml (1)
69-119: DRY up build example sections
The PR and local build examples share identical steps. Consider extracting common steps or referencing a snippet to reduce duplication.src/roles/kserve-tests/e2e-test/main.sh (2)
30-35: Guardcdcommands against failure
Add|| exit 1after eachcdto abort on failure:- cd "$ROLE_DIR" + cd "$ROLE_DIR" || exit 1 - cd opendatahub-tests + cd opendatahub-tests" || exit 1
43-43: SplitDOMAINdeclaration and export
Avoid masking return codes by separating assignment from export:DOMAIN=$(oc get ingresses.config.openshift.io cluster -o jsonpath='{.spec.domain}') export DOMAIN
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/roles/kserve-tests/e2e-test/main.sh(1 hunks)src/roles/minio/deploy/main.sh(2 hunks)unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
src/roles/kserve-tests/e2e-test/main.sh
[warning] 28-28: role_name appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 30-30: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 37-37: result appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 43-43: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 46-46: You are missing a required space here.
(SC1035)
src/roles/minio/deploy/main.sh
[warning] 72-72: Quote this to prevent word splitting.
(SC2046)
[warning] 75-75: edge_enable_ssl is referenced but not assigned.
(SC2154)
🪛 YAMLlint (1.37.1)
unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 94-94: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cluster-test (3.12)
🔇 Additional comments (3)
src/roles/minio/deploy/main.sh (1)
66-68: Clarify intentional suppression of SSL-false error handling
Theelsebranch for disabled SSL comments out the error stop, which may hide misconfigurations. Confirm that suppressing failures here is intended, or restore the error logic.unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml (1)
1-6: Verify playbook naming consistency
Confirm thename: kserve-execute-e2e-test-with-existing-clustermatches the filename and CLI invocation (per PR objective).src/roles/kserve-tests/e2e-test/main.sh (1)
28-28: Audit unused variables
role_nameandresultare set but never referenced later. Remove them or export if required externally.Also applies to: 37-37
| This playbook configure test cluster that install all required objects for e2e tests and run test. | ||
|
|
||
| By default, it will use the fast channel of kserve/odh-model-controller. If you want to test with custom build, you can set the following cases: | ||
|
|
There was a problem hiding this comment.
Remove trailing whitespace
Several lines contain trailing spaces, which YAML lint errors will flag. Please strip trailing whitespace on the indicated lines.
Also applies to: 18-18, 32-32, 35-35, 49-49, 50-50, 78-78, 94-94
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 8-8: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In
unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml
at lines 8, 18, 32, 35, 49, 50, 78, and 94, remove any trailing whitespace
characters at the end of these lines to fix YAML lint errors. Ensure no spaces
or tabs remain after the last visible character on these lines.
Signed-off-by: Jooho Lee <jlee@redhat.com>
|
/lgtm |
|
/approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
git clone, and its filesystem effects.Chores
uvandpytest_jirato the requirements list.Style
Refactor
Documentation