Conversation
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
WalkthroughThis change introduces a complete support suite for managing Kubernetes-in-Docker (kind) clusters, including installation and uninstallation roles, configuration, and end-to-end (E2E) tests. It adds utility functions, updates environment setup scripts, and transitions development tooling from Python to Go. Marker and configuration improvements are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Loopy CLI
participant Kind Install Role
participant Kind CLI
participant Kubernetes Cluster
Tester->>Loopy CLI: Run "kind-install" role
Loopy CLI->>Kind Install Role: Set up environment, parse config
Kind Install Role->>Kind CLI: Create cluster with config
Kind CLI->>Kubernetes Cluster: Provision cluster, apply ingress/OLM if enabled
Kind CLI-->>Kind Install Role: Return status
Kind Install Role-->>Loopy CLI: Output KUBECONFIG_PATH, status
Loopy CLI-->>Tester: Report result
Tester->>Loopy CLI: Run "kind-uninstall" role
Loopy CLI->>Kind Uninstall Role: Set up environment, parse config
Kind Uninstall Role->>Kind CLI: Delete cluster by name
Kind CLI-->>Kind Uninstall Role: Return status
Kind Uninstall Role-->>Loopy CLI: Report deletion status
Loopy CLI-->>Tester: Report result
Possibly related PRs
Poem
✨ 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: 7
🧹 Nitpick comments (10)
src/commons/scripts/utils.sh (1)
515-535: Renamecommandvariable and hardenretry()
- The local variable name
commandshadows the Bash builtin of the same name. Renaming it tocmdavoids confusion and potential conflicts.- Consider validating that
max_attemptsandsleep_secondsare positive integers before using them to prevent silent infinite loops or errors.- Wrap messages in quotes consistently to handle cases with special characters.
Example diff:
- local max_attempts=$1 - local sleep_seconds=$2 - local command=$3 - local message=$4 + local max_attempts="$1" + local sleep_seconds="$2" + local cmd="$3" + local message="$4" local attempt=1 - until eval "$command"; do + until eval "$cmd"; do if (( attempt == max_attempts )); then - echo "Failed after $max_attempts attempts: $message" + echo "Failed after ${max_attempts} attempts: ${message}" return 1 fi - echo "Attempt $attempt/$max_attempts: $message" + echo "Attempt ${attempt}/${max_attempts}: ${message}" sleep "$sleep_seconds" (( attempt++ )) done return 0hacks/download-cli.sh (1)
172-180: Use the existingdelete_if_existshelper for consistency
The block for installingkindduplicates the manualrm -rfpattern already encapsulated indelete_if_exists. Leveraging that function improves readability and unifies cleanup logic.Proposed diff:
- rm -rf "${root_directory}/bin/kind" + delete_if_exists "${root_directory}/bin/kind"pytest.ini (1)
30-33: Consider renaming the new marker for consistency
You addedcluster_life_cycle_testsalongside markers likecluster_tests. For readability, you might rename it tocluster_lifecycle_tests(no extra underscore) or evencluster_lifecycleto match the concise style of other markers.src/roles/kind/install/files/kind-ingress-config.yaml (1)
10-10: Add a newline at end of file
YAML lint flagged a missing newline. Please ensure the file ends with a single blank line to satisfy thenew-line-at-end-of-filerule.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
.vscode/launch.json (1)
27-29: Avoid hardcoding user-specificKUBEBUILDER_ASSETSpaths
Both Ginkgo configurations reference/home/jooho/..., which won’t work for other developers. Use a VSCode or shell environment variable (e.g.${env:HOME}or a custom${env:KUBEBUILDER_ASSETS}) to make this portable.Example diff:
- "KUBEBUILDER_ASSETS": "/home/jooho/.local/share/kubebuilder-envtest/k8s/1.29.5-linux-amd64" + "KUBEBUILDER_ASSETS": "${env:KUBEBUILDER_ASSETS:-${env:HOME}/.local/share/kubebuilder-envtest/k8s/1.29.5-linux-amd64}"Also applies to: 37-39
src/roles/kind/install/config.yaml (1)
10-10: Fix trailing spaces.Remove the trailing spaces at the end of line 10.
- Docker/Podman installed + Docker/Podman installed🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 10-10: trailing spaces
(trailing-spaces)
src/roles/kind/uninstall/main.sh (1)
29-29: Remove unused variable.The
role_namevariable is extracted but never used in the script. Consider removing it to clean up the code.-role_name=$(yq e '.role.name' ${current_dir}/config.yaml)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 29-29: role_name appears unused. Verify use (or export if used externally).
(SC2034)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py (2)
1-1: Add module docstring.Add a module docstring to describe the purpose of this test fixture module.
+"""Test fixtures for kind cluster lifecycle e2e tests.""" + import pytest🧰 Tools
🪛 Ruff (0.11.9)
1-1:
osimported but unusedRemove unused import:
os(F401)
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[warning] 1-1: Unused import os
(W0611)
58-62: Use more specific exception handling.Instead of catching the general
Exception, catch more specific exceptions likeOSErrororPermissionErrorfor file operations.- except Exception as e: + except (OSError, PermissionError) as e:🧰 Tools
🪛 Pylint (3.3.7)
[warning] 61-61: Catching too general exception Exception
(W0718)
[convention] 58-58: Import outside toplevel (shutil)
(C0415)
tests/e2e/cluster-life-cycle/kind_install_delete/test_kind.py (1)
19-32: Add explicitcheckparameter to subprocess.run calls.For better error handling and to address static analysis warnings, explicitly set the
checkparameter in subprocess.run calls.result = subprocess.run( [ "./loopy", "roles", "run", "kind-install", "-l", "-g", "-r", ], capture_output=True, text=True, + check=False, env=env, )Apply the same change to the second subprocess.run call in the deletion test.
Also applies to: 73-86
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 19-32: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.vscode/launch.json(1 hunks)hacks/download-cli.sh(1 hunks)pytest.ini(1 hunks)src/commons/scripts/utils.sh(1 hunks)src/roles/kind/__init__.py(1 hunks)src/roles/kind/install/__init__.py(1 hunks)src/roles/kind/install/config.yaml(1 hunks)src/roles/kind/install/files/kind-ingress-config.yaml(1 hunks)src/roles/kind/install/main.sh(1 hunks)src/roles/kind/uninstall/__init__.py(1 hunks)src/roles/kind/uninstall/config.yaml(1 hunks)src/roles/kind/uninstall/main.sh(1 hunks)tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py(1 hunks)tests/e2e/cluster-life-cycle/kind_install_delete/test_kind.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/e2e/cluster-life-cycle/kind_install_delete/test_kind.py (1)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py (1)
role_env(27-34)
src/roles/kind/uninstall/main.sh (1)
src/commons/scripts/utils.sh (4)
info(59-61)fail(75-78)success(86-88)retry(518-535)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py (1)
tests/fvt/roles/conftest.py (1)
output_root_dir(8-10)
🪛 YAMLlint (1.37.1)
src/roles/kind/install/files/kind-ingress-config.yaml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
src/roles/kind/install/config.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
🪛 Pylint (3.3.7)
tests/e2e/cluster-life-cycle/kind_install_delete/test_kind.py
[convention] 1-1: Missing module docstring
(C0114)
[warning] 5-5: String statement has no effect
(W0105)
[warning] 19-32: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
[warning] 73-86: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
[convention] 2-2: standard import "subprocess" should be placed before third party import "pytest"
(C0411)
[convention] 3-3: standard import "os" should be placed before third party import "pytest"
(C0411)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py
[convention] 1-1: Missing module docstring
(C0114)
[warning] 27-27: Redefining name 'base_env' from outer scope (line 10)
(W0621)
[convention] 38-38: Missing function or method docstring
(C0116)
[warning] 38-38: Redefining name 'base_env' from outer scope (line 10)
(W0621)
[warning] 47-47: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
[warning] 61-61: Catching too general exception Exception
(W0718)
[convention] 58-58: Import outside toplevel (shutil)
(C0415)
[convention] 4-4: standard import "pathlib.Path" should be placed before third party imports "pytest", "yaml"
(C0411)
[convention] 5-5: standard import "subprocess" should be placed before third party imports "pytest", "yaml"
(C0411)
[convention] 6-6: standard import "uuid" should be placed before third party imports "pytest", "yaml"
(C0411)
[warning] 1-1: Unused import os
(W0611)
[warning] 3-3: Unused import yaml
(W0611)
🪛 Shellcheck (0.10.0)
src/roles/kind/uninstall/main.sh
[warning] 29-29: role_name appears unused. Verify use (or export if used externally).
(SC2034)
🪛 Ruff (0.11.9)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py
1-1: os imported but unused
Remove unused import: os
(F401)
3-3: yaml imported but unused
Remove unused import: yaml
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cluster-test (3.12)
🔇 Additional comments (8)
src/commons/scripts/utils.sh (1)
512-512: Explicit success return invalidate_script_params
Addingreturn 0at the end ofvalidate_script_paramsmakes the function’s intent clearer by explicitly indicating success. This improves readability and avoids relying on implicit return values.src/roles/kind/__init__.py (1)
1-1: Approve new package initializer
The single-line docstring clearly defines thekindrole package. No further changes needed.src/roles/kind/install/__init__.py (1)
1-1: Approve new install package initializer
The docstring succinctly describes thekind.installrole package.src/roles/kind/uninstall/__init__.py (1)
1-1: Approve new uninstall package initializer
The docstring is appropriate for thekind.uninstallrole package.src/roles/kind/install/main.sh (1)
23-23: A blank line was added for readability. No functional impact to review..vscode/launch.json (1)
1-43: VSCode launch configuration updated for Go workflows
The new Go “Launch Package” and Ginkgo test configurations align well with the Go-based tooling introduced in this PR.src/roles/kind/uninstall/main.sh (2)
1-25: LGTM: Well-structured script initialization.The script initialization properly handles debug mode, finds the repository root, and sources utility functions. The error handling for finding the .git directory is appropriate.
31-64: LGTM: Robust uninstall and verification logic.The script properly:
- Checks for kind CLI availability
- Handles the case where cluster doesn't exist (not an error)
- Uses retry mechanism to verify cluster deletion
- Provides appropriate logging and error handling
- Reports results in the expected format
The use of the retry function with a 2-minute timeout is a good practice for ensuring cluster deletion is complete.
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
|
/approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/roles/kind/install/config.yaml (1)
14-14: Fix version format inconsistency.There's an inconsistency in the NGINX_INGRESS_VERSION format across the file. The usage example uses "v1.10.1" (with "v" prefix) while the description and default value use "1.10.1" (without "v" prefix).
For consistency, update the description and default to include the "v" prefix:
- name: NGINX_INGRESS_VERSION description: | - Set NGINX_INGRESS_VERSION (default: 1.10.1) + Set NGINX_INGRESS_VERSION (default: v1.10.1) - default: 1.10.1 + default: v1.10.1Also applies to: 20-20, 44-44
🧹 Nitpick comments (8)
src/roles/kind/install/config.yaml (1)
10-10: Fix trailing spaces.There are trailing spaces at the end of line 10 that should be removed.
- Docker/Podman installed + Docker/Podman installed🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 10-10: trailing spaces
(trailing-spaces)
tests/e2e/cluster-life-cycle/kind_install_delete/test_kind.py (2)
16-29: Add explicit error handling for subprocess calls.The
subprocess.runcalls don't specify thecheckparameter, which means they won't automatically raise exceptions on failure. While you're manually checkingresult.returncode, it's better to be explicit about error handling strategy.Consider one of these approaches:
Option 1: Use
check=Truefor automatic exception handling:result = subprocess.run( [ "./loopy", "roles", "run", "kind-install", "-l", "-g", "-r", ], capture_output=True, text=True, env=env, + check=True, ) -assert result.returncode == 0, f"Command failed: {result.stderr}"Option 2: Explicitly set
check=Falseto document the intent:result = subprocess.run( [ "./loopy", "roles", "run", "kind-install", "-l", "-g", "-r", ], capture_output=True, text=True, env=env, + check=False, ) assert result.returncode == 0, f"Command failed: {result.stderr}"Also applies to: 68-81
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 16-29: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
86-86: Add missing final newline.The file is missing a final newline, which is a Python convention.
assert f"{role_env['KIND_CLUSTER_NAME']}" not in clusters, "Cluster was not deleted" +🧰 Tools
🪛 Pylint (3.3.7)
[convention] 86-86: Final newline missing
(C0304)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py (5)
1-1: Add module docstring.The module is missing a docstring that should describe its purpose.
+"""Pytest fixtures for kind cluster lifecycle end-to-end tests.""" + import pytest🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
1-4: Fix import order.Standard library imports should come before third-party imports according to Python conventions.
+from pathlib import Path +import subprocess +import uuid + import pytest -from pathlib import Path -import subprocess -import uuid🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[convention] 2-2: standard import "pathlib.Path" should be placed before third party import "pytest"
(C0411)
[convention] 3-3: standard import "subprocess" should be placed before third party import "pytest"
(C0411)
[convention] 4-4: standard import "uuid" should be placed before third party import "pytest"
(C0411)
25-25: Consider renaming fixture parameters to avoid shadowing.The
base_envparameter in bothrole_envandcleanupfixtures shadows the fixture name, which can be confusing. Consider using more descriptive parameter names.@pytest.fixture() -def role_env(base_env): +def role_env(base_env_dict): """Fixture providing base environment variables needed for all tests""" - base_env["KIND_CLUSTER_NAME"] = "kind" + base_env_dict["KIND_CLUSTER_NAME"] = "kind" # ... rest of the function - return base_env + return base_env_dict @pytest.fixture(scope="session", autouse=True) -def cleanup(base_env): +def cleanup(base_env_dict): + """Cleanup fixture that removes clusters and output directories after tests.""" yield # ... rest of the function - cleanup_commands = [ - f"kind delete cluster --name {base_env['KIND_CLUSTER_NAME']}", - ] + cleanup_commands = [ + f"kind delete cluster --name {base_env_dict['KIND_CLUSTER_NAME']}", + ]Also applies to: 36-36
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 25-25: Redefining name 'base_env' from outer scope (line 8)
(W0621)
45-45: Add explicit subprocess error handling.The
subprocess.runcall doesn't specify thecheckparameter. Since you're handling errors manually, make this explicit.- result = subprocess.run(cmd, capture_output=True, text=True, shell=True) + result = subprocess.run(cmd, capture_output=True, text=True, shell=True, check=False)🧰 Tools
🪛 Pylint (3.3.7)
[warning] 45-45: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
56-60: Improve exception handling and import placement.The broad exception handling and import inside the function can be improved.
Move the import to the top of the file and make exception handling more specific:
+import shutil from pathlib import Path import subprocess import uuid # ... later in cleanup function ... if output_root_dir.exists(): try: - import shutil - shutil.rmtree(output_root_dir) - except Exception as e: - print(f"Warning: Failed to clean up output directory: {e}") + except (OSError, PermissionError) as e: + print(f"Warning: Failed to clean up output directory {output_root_dir}: {e}")🧰 Tools
🪛 Pylint (3.3.7)
[warning] 59-59: Catching too general exception Exception
(W0718)
[convention] 56-56: Import outside toplevel (shutil)
(C0415)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/roles/kind/install/config.yaml(1 hunks)src/roles/kind/install/main.sh(2 hunks)tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py(1 hunks)tests/e2e/cluster-life-cycle/kind_install_delete/test_kind.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/roles/kind/install/main.sh
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py (1)
tests/fvt/roles/conftest.py (1)
output_root_dir(8-10)
tests/e2e/cluster-life-cycle/kind_install_delete/test_kind.py (1)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py (1)
role_env(25-32)
🪛 YAMLlint (1.37.1)
src/roles/kind/install/config.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
🪛 Pylint (3.3.7)
tests/e2e/cluster-life-cycle/kind_install_delete/conftest.py
[convention] 1-1: Missing module docstring
(C0114)
[warning] 25-25: Redefining name 'base_env' from outer scope (line 8)
(W0621)
[convention] 36-36: Missing function or method docstring
(C0116)
[warning] 36-36: Redefining name 'base_env' from outer scope (line 8)
(W0621)
[warning] 45-45: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
[warning] 59-59: Catching too general exception Exception
(W0718)
[convention] 56-56: Import outside toplevel (shutil)
(C0415)
[convention] 2-2: standard import "pathlib.Path" should be placed before third party import "pytest"
(C0411)
[convention] 3-3: standard import "subprocess" should be placed before third party import "pytest"
(C0411)
[convention] 4-4: standard import "uuid" should be placed before third party import "pytest"
(C0411)
tests/e2e/cluster-life-cycle/kind_install_delete/test_kind.py
[convention] 86-86: Final newline missing
(C0304)
[warning] 16-29: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
[warning] 68-81: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
1 participant
Creating a kind cluster is the first requirement for testing upstream KServe. To streamline this process and avoid individual setup overhead for each team member, we should provide a dedicated Loopy role that automates the creation of a kind cluster.
This role will allow developers to easily provision a local Kubernetes environment tailored for KServe testing without worrying about the underlying setup steps.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
Refactor