Repository navigation
feat: Add test configuration and execution tool - #321
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Typer CLI (mtv-api-tests) with interactive Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as mtv-api-tests CLI
participant RichUI as Rich Prompts
participant vSphere
participant OpenShift
participant FileSystem as File System
User->>CLI: uv run mtv-api-tests generate
CLI->>RichUI: Prompt for category & filters
RichUI-->>CLI: category, filter
rect rgba(200,150,100,0.5)
Note over CLI,OpenShift: OpenShift discovery
CLI->>OpenShift: Connect & auth (kubeconfig/API creds)
OpenShift-->>CLI: OCP version, StorageClasses, detect MTV
CLI->>RichUI: Prompt select storage class
RichUI-->>CLI: storage_class
end
rect rgba(100,150,200,0.5)
Note over CLI,vSphere: vSphere discovery
CLI->>vSphere: Connect & auth
vSphere-->>CLI: version, datastores, VMs, hosts
CLI->>RichUI: Prompt select datastores, VM, host, vendor fields
RichUI-->>CLI: selections & credentials
end
rect rgba(150,200,100,0.5)
Note over CLI,FileSystem: Config generation
CLI->>CLI: Build providers.json and Job YAML
CLI->>FileSystem: Atomic write .providers.json (0o600)
CLI->>FileSystem: Write mtv-api-tests-manifests.yaml
end
CLI->>RichUI: Show next-steps (local/job run commands)
RichUI->>User: Display commands
sequenceDiagram
participant User
participant CLI as mtv-api-tests run
participant FileSystem as File System
participant Provider as Providers.json
participant OCP as oc / OpenShift
participant Local as uv pytest
User->>CLI: uv run mtv-api-tests run --mode local --category copyoffload
CLI->>FileSystem: Read .providers.json
FileSystem-->>CLI: providers data
CLI->>Provider: Validate providers and types
CLI->>CLI: Select source (non-openshift) & destination (openshift)
rect rgba(100,150,200,0.5)
Note over CLI,OCP: Cluster auth resolution
alt provider has host/creds
CLI->>OCP: Use provider host/user/pass as --tc env
else kubeconfig/token available
CLI->>OCP: oc whoami -t
OCP-->>CLI: token
else
CLI->>User: Prompt for API/creds
end
end
rect rgba(150,200,100,0.5)
Note over CLI,Local: Local execution
CLI->>Local: Build pytest command (uv run pytest --tc ... -m ... -k ...)
Local-->>CLI: Run and return exit code
end
CLI->>User: Exit with pytest return code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Review Summary by QodoAdd interactive MTV test configuration and execution CLI tool
WalkthroughsDescription• Add interactive CLI tool for MTV test configuration and execution - Discovers vSphere/OpenShift resources via APIs - Generates .providers.json and self-contained job.yaml - Supports local and OpenShift Job test execution modes • Modernize Dockerfile with UBI-minimal base and improved security - Switch from Fedora to UBI-minimal for production readiness - Add non-root user (1001) and security context settings - Improve Python environment variables for logging and caching • Add Makefile with convenient targets for common operations • Document interactive setup tool in README and copyoffload guide Diagramflowchart LR
A["User"] -->|"make generate"| B["mtv-api-tests.py<br/>generate command"]
B -->|"Discover resources"| C["vSphere API"]
B -->|"Discover resources"| D["OpenShift API"]
C -->|"Return datastores,<br/>VMs, hosts"| E["Interactive Wizard"]
D -->|"Return storage<br/>classes"| E
E -->|"Gather credentials<br/>& selections"| F[".providers.json<br/>+ job.yaml"]
F -->|"make run-local"| G["Local pytest<br/>execution"]
F -->|"make run-job"| H["oc apply<br/>OpenShift Job"]
File Changes1. tools/mtv-api-tests.py
|
Code Review by Qodo
1. OCP host field required
|
rgolangh
changed the title
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
rgolangh
force-pushed
the
helper-to-create-manifests
branch
from
March 9, 2026 11:19
8faaa79 to
ec88acd
Compare
|
Persistent review updated to latest commit ec88acd |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 34: The Dockerfile currently uses a blanket COPY . . which bakes
generated secrets (e.g., .providers.json and job.yaml created by
tools/mtv-api-tests.py) into the image; replace this with explicit COPY
instructions for only the files needed by the image (reference the COPY . . line
in the Dockerfile) and add a .dockerignore that excludes generated artifacts
such as .providers.json, job.yaml, and any other test-generated outputs to
prevent secrets from entering the build context or final image.
In `@README.md`:
- Around line 448-483: The README's "Interactive Setup Tool" docs do not warn
that the generated job.yaml contains a Kubernetes Secret with provider and
cluster credentials; update the section describing job.yaml (referencing
tools/mtv-api-tests.py generate, job.yaml, and .providers.json) to explicitly
state that job.yaml embeds secrets, advise not to commit it to version control,
recommend restricting file permissions and deleting it after use, and mirror the
same cautionary language currently used for .providers.json so users understand
the sensitive nature of the file.
In `@tools/mtv-api-tests.py`:
- Around line 632-633: The generated secret files are written with
Path.write_text which inherits the process umask; after writing
PROVIDERS_JSON_PATH (and the equivalent job.yaml path), immediately set
restrictive permissions with a chmod to 0o600 so only the owner can read/write;
update the code around the PROVIDERS_JSON_PATH.write_text call to follow it with
PROVIDERS_JSON_PATH.chmod(0o600) and do the same for the job.yaml path (use its
variable name in the diff) to ensure secrets are not world-readable.
- Around line 679-680: The code constructs filter_flag by interpolating
user-controlled test_filter directly into a shell command; update the
construction to shell-quote the value (use shlex.quote(test_filter)) before
embedding so single quotes and metacharacters are escaped, e.g. replace f"-k
'{test_filter}' " with f"-k {shlex.quote(test_filter)} " and add the necessary
import for shlex; apply the same change to the other occurrence around lines
742-748 that builds a similar filter_flag.
- Around line 968-969: The generate step collects OCP credentials via
_gather_ocp_storage_class() but run --mode local discards ocp_creds and only
passes source_provider/storage_class, so local runs fail if the user provided
kubeconfig/host+user+pass interactively; update the flow so that generate
persists the OCP auth inputs (kubeconfig path or host/username/password and/or
SA token) into the produced artifacts (e.g., extend .providers.json or job.yaml
with an ocp_creds block) and change the run (--mode local) path to load and
reuse that ocp_creds when launching pytest, or alternatively make run --mode
local re-prompt for missing cluster credentials by invoking the same
_gather_ocp_storage_class() logic before starting tests; target references:
_gather_ocp_storage_class(), generate step that writes .providers.json/job.yaml,
and the run-local path that launches pytest.
- Around line 1047-1052: The code calls generate() when the requested job_yaml
path is missing but generate() always writes the default "job.yaml", causing
_run_job(Path(job_yaml)) to still fail for a custom --job-yaml; modify the flow
so that when mode == RunMode.job and the provided job_yaml path does not exist
you either call generate(target_path=job_yaml) (i.e. make generate accept an
output path and write the generated YAML to that custom path) or after calling
generate() update job_yaml to the generated default path (e.g., set job_yaml =
"job.yaml") before calling _run_job(Path(job_yaml)), ensuring _run_job is
invoked against the file that was actually created; update the generate()
signature/usage and any callers accordingly or adjust the post-generate job_yaml
reassignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a81e1a7-8dae-4445-bc54-5962dd7189f3
📒 Files selected for processing (5)
DockerfileMakefileREADME.mddocs/copyoffload/how-to-run-copyoffload-tests.mdtools/mtv-api-tests.py
|
Persistent review updated to latest commit ec88acd |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
Dockerfile (1)
34-34:⚠️ Potential issue | 🔴 CriticalCRITICAL:
COPY . .will bake generated secrets into the image.With this blanket copy, anyone who runs
make generatebeforemake build-imageships.providers.jsonandjob.yaml(which contain credentials) into the build context and final image. Add a.dockerignoreto exclude these files.📝 Create `.dockerignore`
.providers.json job.yaml .env .git/ .cache/ output/ __pycache__/ *.pyc .data-collector/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 34, The Dockerfile uses a blanket COPY . . which bakes generated secrets (like .providers.json and job.yaml) into the image; create a .dockerignore at repo root to exclude those files and other build artifacts so they never enter the build context (exclude .providers.json, job.yaml, .env, .git/, .cache/, output/, __pycache__/, *.pyc, .data-collector/ as suggested) and then keep the existing COPY . . in Dockerfile unchanged so only safe files are copied during make build-image after make generate.tools/mtv-api-tests.py (3)
675-677:⚠️ Potential issue | 🟠 MajorHIGH: Shell-quote
test_filterbefore embedding into bash command.
test_filteris user input spliced directly into a shell script that runs inside the Job. A filter containing'breaks the command; shell metacharacters can escape the pytest invocation entirely.🔧 Proposed fix
+import shlex ... - filter_flag = f"-k '{test_filter}' " if test_filter else "" + filter_flag = f"-k {shlex.quote(test_filter)} " if test_filter else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/mtv-api-tests.py` around lines 675 - 677, The filter_flag currently interpolates raw user input test_filter into a shell command; to prevent injection and broken quoting, import and use shlex.quote to shell-escape test_filter and build filter_flag from the escaped value (e.g. replace the current filter_flag = f"-k '{test_filter}' " with a version that uses shlex.quote(test_filter)); update places that construct or pass filter_flag accordingly so the command string always contains the safely quoted filter.
631-633:⚠️ Potential issue | 🟠 MajorHIGH: Write generated secret files with
0600permissions.Both
.providers.jsonandjob.yamlcontain live vSphere/OpenShift credentials.Path.write_text()inherits the process umask, so on a common022shell these files become world-readable. Lock them down immediately after writing.🔒 Proposed fix
PROVIDERS_JSON_PATH.write_text(json.dumps(existing, indent=2) + "\n") + PROVIDERS_JSON_PATH.chmod(0o600) console.print(f" [green]Wrote {PROVIDERS_JSON_PATH}[/green]")Also applies to line 1016-1017 for
JOB_YAML_PATH.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/mtv-api-tests.py` around lines 631 - 633, After writing the sensitive files, immediately tighten their permissions instead of relying on the process umask: after the existing PROVIDERS_JSON_PATH.write_text(...) call, add PROVIDERS_JSON_PATH.chmod(0o600) to set owner read/write only, and do the same after JOB_YAML_PATH.write_text(...) (add JOB_YAML_PATH.chmod(0o600)). This ensures .providers.json and job.yaml are locked down to mode 0600 regardless of the umask.
1045-1050:⚠️ Potential issue | 🟠 MajorHIGH: Auto-generate fallback is wrong for a custom
--job-yamlpath.
run --mode job --job-yaml custom.yamlcallsgenerate()when the file is missing, butgenerate()always writes to the defaultjob.yaml. The command then tries to applycustom.yamlwhich still doesn't exist.🔧 Proposed fix
if mode == RunMode.job: - if not Path(job_yaml).exists(): - console.print("[yellow]job.yaml not found. Starting configuration wizard...[/yellow]\n") - generate() - _run_job(Path(job_yaml)) + job_yaml_path = Path(job_yaml) + if not job_yaml_path.exists(): + if job_yaml_path != JOB_YAML_PATH: + console.print(f"[red]Error: {job_yaml_path} not found.[/red]") + raise typer.Exit(code=1) + console.print("[yellow]job.yaml not found. Starting configuration wizard...[/yellow]\n") + generate() + _run_job(job_yaml_path) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/mtv-api-tests.py` around lines 1045 - 1050, The code calls generate() when a custom job_yaml is missing but generate() always writes the default "job.yaml", so the requested custom file remains absent; change the logic to either error or make generate write to the requested path: update generate() to accept a target path/output filename (e.g., generate(target_path: Path)) and in the RunMode.job branch replace generate() with generate(Path(job_yaml)) (or, if you prefer failing behavior, raise a clear error when job_yaml != "job.yaml" and the file is missing); ensure _run_job(Path(job_yaml)) then runs against the newly created file.README.md (1)
448-469:⚠️ Potential issue | 🟠 MajorHIGH: Add security warning that
job.yamlcontains embedded secrets.The section introduces
job.yamlas part of the workflow but doesn't warn users that it embeds a KubernetesSecretwith base64-encoded provider data and cluster credentials. It should receive the same "don't commit / restrict permissions / delete when done" guidance as.providers.json.📝 Suggested addition after line 468
6. Write `.providers.json` and `job.yaml` +> ⚠️ **Security note**: `job.yaml` contains a Kubernetes `Secret` with base64-encoded +> provider configuration and cluster credentials. Treat it like `.providers.json`: +> do not commit it to version control, restrict file permissions (`chmod 600 job.yaml`), +> and delete it when you are done.As per coding guidelines:
README.md: Update README.md when code changes affect usage, requirements, installation, or configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 448 - 469, Add a security warning after the interactive wizard description noting that job.yaml (generated by tools/mtv-api-tests.py) contains an embedded Kubernetes Secret with base64-encoded provider data and cluster credentials; instruct users not to commit job.yaml (or .providers.json) to VCS, to restrict filesystem and repository access, to delete the files when no longer needed, and to treat them the same way as .providers.json regarding permissions and rotation of credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 30: The RUN pip install uv line installs an unpinned package; update the
Dockerfile to pin the uv package to a tested version (e.g., replace the bare
"uv" with "uv==<your-tested-version>") and add the --no-cache-dir flag to the
pip install invocation to avoid caching the wheel and keep the image small;
modify the RUN pip install uv invocation accordingly (use the chosen version
number in place of <your-tested-version>).
In `@tools/mtv-api-tests.py`:
- Around line 79-84: Replace the current RunMode definition that inherits from
(str, Enum) with Python 3.11+'s StrEnum: import StrEnum from enum and change the
class signature to inherit from StrEnum (class RunMode(StrEnum):) while keeping
the same members local = "local" and job = "job"; update any type references if
necessary to reflect StrEnum usage and ensure the file imports enum.StrEnum
instead of Enum.
- Around line 374-376: Update the exception handling around the get_client()
call: instead of raising a new ConnectionError without chaining, re-raise it
using exception chaining (raise ConnectionError(...) from exc) so the original
exception is preserved; locate the except Exception as exc block that currently
raises ConnectionError(f"Failed to connect to OpenShift: {exc}") and change it
to chain the original exception (using the same message) to aid debugging while
keeping the bare Exception catch.
- Around line 222-228: The except handlers around the SmartConnect call should
chain the original exceptions into the new ConnectionError so the traceback is
preserved; update the vim.fault.InvalidLogin and generic except Exception as exc
branches to re-raise ConnectionError using "raise ConnectionError(... ) from
exc" (use the exc variable in both places) so callers see the causal chain from
the original SmartConnect/vim.fault.InvalidLogin failure to the ConnectionError.
- Around line 1119-1134: Validate the user-controlled values provider_key and
storage_class before building cmd and calling subprocess.run to prevent
malformed or unsafe values; in the function or block that constructs cmd
(referencing variables provider_key, storage_class, cmd and the subprocess.run
invocation), add a small sanitizer/validator that rejects or normalizes values
containing unexpected characters (e.g., whitespace, shell metacharacters, path
separators) or check them against an allowlist of expected provider/storage
names, and raise a clear error or prompt the user if validation fails before
proceeding to run pytest.
- Around line 778-781: When handling ConnectionError from connect_vsphere,
preserve the original exception context before exiting: call
console.print_exception() (or log the traceback) to emit the full traceback,
then raise typer.Exit(...) using exception chaining (raise typer.Exit(code=1)
from exc) so the underlying ConnectionError is linked; apply the same change to
the other identical handler around lines 898-901. Ensure you reference the
connect_vsphere caller, the console.print_exception()/console.print usage, and
the typer.Exit raise so the traceback is preserved for diagnostics.
---
Duplicate comments:
In `@Dockerfile`:
- Line 34: The Dockerfile uses a blanket COPY . . which bakes generated secrets
(like .providers.json and job.yaml) into the image; create a .dockerignore at
repo root to exclude those files and other build artifacts so they never enter
the build context (exclude .providers.json, job.yaml, .env, .git/, .cache/,
output/, __pycache__/, *.pyc, .data-collector/ as suggested) and then keep the
existing COPY . . in Dockerfile unchanged so only safe files are copied during
make build-image after make generate.
In `@README.md`:
- Around line 448-469: Add a security warning after the interactive wizard
description noting that job.yaml (generated by tools/mtv-api-tests.py) contains
an embedded Kubernetes Secret with base64-encoded provider data and cluster
credentials; instruct users not to commit job.yaml (or .providers.json) to VCS,
to restrict filesystem and repository access, to delete the files when no longer
needed, and to treat them the same way as .providers.json regarding permissions
and rotation of credentials.
In `@tools/mtv-api-tests.py`:
- Around line 675-677: The filter_flag currently interpolates raw user input
test_filter into a shell command; to prevent injection and broken quoting,
import and use shlex.quote to shell-escape test_filter and build filter_flag
from the escaped value (e.g. replace the current filter_flag = f"-k
'{test_filter}' " with a version that uses shlex.quote(test_filter)); update
places that construct or pass filter_flag accordingly so the command string
always contains the safely quoted filter.
- Around line 631-633: After writing the sensitive files, immediately tighten
their permissions instead of relying on the process umask: after the existing
PROVIDERS_JSON_PATH.write_text(...) call, add PROVIDERS_JSON_PATH.chmod(0o600)
to set owner read/write only, and do the same after
JOB_YAML_PATH.write_text(...) (add JOB_YAML_PATH.chmod(0o600)). This ensures
.providers.json and job.yaml are locked down to mode 0600 regardless of the
umask.
- Around line 1045-1050: The code calls generate() when a custom job_yaml is
missing but generate() always writes the default "job.yaml", so the requested
custom file remains absent; change the logic to either error or make generate
write to the requested path: update generate() to accept a target path/output
filename (e.g., generate(target_path: Path)) and in the RunMode.job branch
replace generate() with generate(Path(job_yaml)) (or, if you prefer failing
behavior, raise a clear error when job_yaml != "job.yaml" and the file is
missing); ensure _run_job(Path(job_yaml)) then runs against the newly created
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cd02302-f909-4b79-a2c8-6315e9083111
📒 Files selected for processing (5)
DockerfileMakefileREADME.mddocs/copyoffload/how-to-run-copyoffload-tests.mdtools/mtv-api-tests.py
myakove
requested changes
Mar 9, 2026
myakove
left a comment
myakove
left a comment
Collaborator
There was a problem hiding this comment.
Please split mtv-api-tests.py into smaller logical files
Contributor
Author
give me guidance please |
Collaborator
|
you can use it once merged #323 |
rgolangh
force-pushed
the
helper-to-create-manifests
branch
from
March 9, 2026 18:33
ef268e7 to
de0b7de
Compare
Contributor
Author
|
@coderabbitai[bot] The following review comments were reviewed and a decision was made:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
cli/mtv_api_tests/generate.py (1)
48-62:⚠️ Potential issue | 🟠 MajorHIGH:
_vsphere_session()can still mask the real discovery failure.The recent cleanup refactor moved teardown into a context manager, but
disconnect_vsphere(si)still runs unguarded infinally. If disconnect fails during unwinding, Python replaces the original discovery/selection exception with the disconnect error, which makes the actual failure much harder to diagnose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/generate.py` around lines 48 - 62, The finally block in _vsphere_session currently calls disconnect_vsphere(si) unguarded which can raise and replace the original error from connect_vsphere/usage; wrap the disconnect_vsphere(si) call in its own try/except inside the finally so any exception raised during teardown is caught and logged (using logging.getLogger(__name__).exception or an existing logger) and not re-raised, ensuring the original discovery/selection exception from connect_vsphere or the context body is preserved; reference _vsphere_session, connect_vsphere, and disconnect_vsphere when making the change.cli/mtv_api_tests/common.py (1)
847-879:⚠️ Potential issue | 🟠 MajorHIGH: The generated Job still cannot use
cluster_ca_bundle.Lines 875-878 explicitly note that the PEM is only stored, and Lines 946-979 never mount that key or expose
KUBERNETES_CLIENT_CA_BUNDLEto the container. A cluster that needed a custom CA duringgeneratewill still fail TLS inside the pod.Either wire
cluster_ca_bundleinto the pod when it is present, or fail fast ingenerate_job_yaml()for that configuration so the manifest never advertises an unsupported path.Based on learnings: when reviewing this repository, relevant commit messages should be analyzed explicitly; the recent hardening work preserved the PEM in the Secret, but the pod wiring is still incomplete.
Also applies to: 911-980
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/common.py` around lines 847 - 879, The Secret builder _build_job_secret_data writes cluster_ca_bundle but generate_job_yaml() never mounts or forwards it; either wire cluster_ca_bundle into the pod (add the secret key to the SecretVolume/volumeMount and set the env KUBERNETES_CLIENT_CA_BUNDLE to that mounted path in generate_job_yaml()/Job spec) or detect a non-empty ca_bundle in _build_job_secret_data and fail fast in generate_job_yaml() with a clear error; update the Job manifest generation to mount the secret key path and export KUBERNETES_CLIENT_CA_BUNDLE when ca_bundle is present so TLS succeeds inside the test runner container.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/mtv_api_tests/common.py`:
- Around line 770-797: The function write_providers_json currently only handles
JSON parse errors but will crash with TypeError if the existing file contains
valid JSON whose root is not an object (e.g., list or string); update
write_providers_json to validate the parsed value (the variable existing after
json.loads) and if it's not a dict, log a warning via console.print about
invalid root type for providers_path, set existing = {} to recover, then
continue (so checks that use provider_key and the eventual _write_secret_file
operate on a dict and do not raise TypeError).
- Around line 311-322: get_vsphere_credentials collects ssl_config but those TLS
fields are not being persisted into the provider artifact, so ensure the vSphere
TLS settings survive the generate->run boundary by adding ssl_config into the
provider dict written to .providers.json (modify the code path that serializes
providers/the ".providers.json" writer to include the ssl_config keys obtained
from get_vsphere_credentials), and if the serializer cannot/should not include
these fields, change get_vsphere_credentials to validate and fail fast (raise or
exit) when non-replayable TLS options (e.g., VSPHERE_VERIFY_SSL=false or custom
CA) are provided; update the provider-serialization location that emits
".providers.json" to include the SSL fields (or perform the early failure) so
discovery settings are reproducible.
In `@cli/mtv_api_tests/generate.py`:
- Around line 199-260: _gather_ocp_storage_class() declares it returns
dict[str,str] for job_creds but actually includes non-string types (e.g.,
verify_ssl: bool, ca_bundle could be bytes/str) and may return the original
ocp_creds; update the type signatures and local annotations to reflect
heterogenous values: change the function return type to tuple[dict[str, Any],
str, str] (import Any from typing) and change the local job_creds annotation to
dict[str, Any]; ensure any other variables that assume dict[str,str] are
adjusted to accept the broader dict type (e.g., callers of
_gather_ocp_storage_class should accept dict[str, Any] for job creds).
In `@cli/mtv_api_tests/run.py`:
- Around line 240-248: The current check inspects the entire oc apply output
(combined_output) for the substring "unchanged" which can match non-Job
resources (e.g., Namespace) and incorrectly bail out; update the logic to only
consider "unchanged" status lines that refer to Job resources. Split
combined_output into lines, filter those lines for Job-related status (match
lines containing the resource type "job" or "job.batch" or resource names like
"job/..." — case-insensitive), and only raise typer.Exit(code=1) if any of those
Job-specific lines contain "unchanged"; keep using the existing variables
result.stdout/result.stderr, combined_output and job_yaml_path for context in
the warning message.
- Around line 229-238: Wrap the subprocess.run call that executes ["oc",
"apply", "-f", str(job_yaml_path)] in a try/except that catches
FileNotFoundError and subprocess.TimeoutExpired (same pattern used at line 149),
so a missing oc binary or timeout is handled rather than raising; in the except
block use console.print to show a clear error (include exception message) and
raise typer.Exit(code=1); keep the existing returncode check for non-zero
results and only run it inside the try block to locate this fix around the
subprocess.run invocation in run.py.
- Around line 330-337: The interactive prompts are being invoked before
validating explicit provider keys; move the provider resolution and validation
calls (_resolve_source_provider_key(providers, source_provider) and
_resolve_ocp_provider(providers, destination_provider)) to run before
select_category() and prompt_test_filter(), and if either resolution fails
return/raise a clear error (or exit) so CLI wrappers/CI do not land in the
interactive flow; ensure you still pass the resolved_provider values into later
calls (select_category, prompt_test_filter, test logic) after validation.
In `@conftest.py`:
- Around line 123-131: The teardown currently unconditionally rmtree's
config.option.basetemp; modify pytest_configure (function pytest_configure) to
set a marker (e.g., config._auto_basetemp = True) only when you create the
basetemp automatically, and update the teardown/cleanup path that calls
shutil.rmtree to check that marker (use getattr(session.config,
"_auto_basetemp", False)) before deleting so user-provided --basetemp
directories are never removed by the test session.
In `@utilities/naming.py`:
- Around line 67-72: Remove the unnecessary intermediate variable reassignment
by returning the result of the final transformation directly: avoid assigning
back to `sanitized` before returning and instead return the `re.sub(r"[^\w\-]",
"-", sanitized)` result (using the existing `test_name`/`sanitized` flow) so you
eliminate the redundant `sanitized =` assignment; update the function that
performs the replacements (references: `test_name`, `sanitized`, and the
`re.sub` call) accordingly.
In `@utilities/utils.py`:
- Around line 633-635: Normalize the CLUSTER_VERIFY_SSL environment value before
checking it: when verify_ssl_env is not None, call
verify_ssl_env.strip().lower() and use that normalized value in the existing
membership test so insecure_verify_skip = normalized not in ("true","1","yes");
keep the same variable names (verify_ssl_env, insecure_verify_skip) and lenient
true-set logic to preserve existing behavior.
- Around line 604-640: The code fails to forward CA bundles to the local-mode
client and mis-parses CLUSTER_VERIFY_SSL with trailing whitespace; update
run.py:_get_cluster_config and generate.py to preserve and pass the ca_bundle
into the pytest config, then extend utilities/utils.py (where get_client is
called) to accept and pass a ca_bundle/ca_cert argument into the client creation
logic (add a parameter to get_client/get_cluster_client and use it when
constructing the DynamicClient or HTTP(S) session), and fix CLUSTER_VERIFY_SSL
parsing by calling verify_ssl_env.strip().lower() (instead of
verify_ssl_env.lower()) before comparing to ("true","1","yes") so
whitespace-trimmed values are handled correctly.
---
Duplicate comments:
In `@cli/mtv_api_tests/common.py`:
- Around line 847-879: The Secret builder _build_job_secret_data writes
cluster_ca_bundle but generate_job_yaml() never mounts or forwards it; either
wire cluster_ca_bundle into the pod (add the secret key to the
SecretVolume/volumeMount and set the env KUBERNETES_CLIENT_CA_BUNDLE to that
mounted path in generate_job_yaml()/Job spec) or detect a non-empty ca_bundle in
_build_job_secret_data and fail fast in generate_job_yaml() with a clear error;
update the Job manifest generation to mount the secret key path and export
KUBERNETES_CLIENT_CA_BUNDLE when ca_bundle is present so TLS succeeds inside the
test runner container.
In `@cli/mtv_api_tests/generate.py`:
- Around line 48-62: The finally block in _vsphere_session currently calls
disconnect_vsphere(si) unguarded which can raise and replace the original error
from connect_vsphere/usage; wrap the disconnect_vsphere(si) call in its own
try/except inside the finally so any exception raised during teardown is caught
and logged (using logging.getLogger(__name__).exception or an existing logger)
and not re-raised, ensuring the original discovery/selection exception from
connect_vsphere or the context body is preserved; reference _vsphere_session,
connect_vsphere, and disconnect_vsphere when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82ed0fe3-3d1a-44f6-a1bb-932cda232be1
📒 Files selected for processing (18)
cli/mtv_api_tests/common.pycli/mtv_api_tests/generate.pycli/mtv_api_tests/run.pyconftest.pydocs/automation-and-release.mddocs/container-and-openshift-job-execution.mddocs/development-workflow.mddocs/introduction.mddocs/optional-integrations-and-secrets.mddocs/project-structure.mddocs/pytest-options-and-markers.mddocs/quickstart-first-run.mddocs/runtime-configuration.mddocs/troubleshooting-and-diagnostics.mdpytest.iniutilities/must_gather.pyutilities/naming.pyutilities/utils.py
💤 Files with no reviewable changes (10)
- docs/quickstart-first-run.md
- docs/optional-integrations-and-secrets.md
- pytest.ini
- docs/introduction.md
- docs/development-workflow.md
- docs/runtime-configuration.md
- docs/automation-and-release.md
- docs/project-structure.md
- docs/container-and-openshift-job-execution.md
- docs/troubleshooting-and-diagnostics.md
Contributor
Author
|
@coderabbitai[bot] The following review comments were reviewed and a decision was made:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
If you'd like, I can open a follow-up GitHub issue to track the CA bundle consumption gap so it doesn't get lost after this PR merges. Just say the word. 😌 🧠 Learnings used |
Contributor
Author
|
/verified |
1 similar comment
Contributor
Author
|
/verified |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
utilities/utils.py (1)
633-635:⚠️ Potential issue | 🟠 MajorHIGH: Normalize
CLUSTER_VERIFY_SSLbefore evaluating it.For the
feat: Add test configuration and execution toolflow, this env var is now user-facing. Values like"true "or"YES\n"currently fall through toinsecure_verify_skip=Trueand silently disable TLS verification. Keep the existing lenient true-set, but normalize withstrip().lower()first.Suggested fix
verify_ssl_env = os.environ.get("CLUSTER_VERIFY_SSL") if verify_ssl_env is not None: - insecure_verify_skip = verify_ssl_env.lower() not in ("true", "1", "yes") + insecure_verify_skip = verify_ssl_env.strip().lower() not in ("true", "1", "yes")Based on learnings:
CLUSTER_VERIFY_SSLparsing here is intentionally lenient, but it should be case-insensitive afterstrip().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/utils.py` around lines 633 - 635, The CLUSTER_VERIFY_SSL env var parsing is too permissive—normalize its value by calling strip().lower() on verify_ssl_env before checking membership so values like "true ", " YES\n" are recognized; update the logic that sets insecure_verify_skip (using the verify_ssl_env variable) to use the normalized string and keep the same truthy set ("true","1","yes") while preserving the current inverse assignment (insecure_verify_skip = normalized not in (...)).cli/mtv_api_tests/generate.py (1)
199-260: 🛠️ Refactor suggestion | 🟠 MajorHIGH: Fix the
job_credstype contract.This is still annotated as
dict[str, str], but the function adds non-string TLS fields (verify_ssl,ca_bundle) and can return the original heterogeneous credential map. That makes the new generate flow’s type contract false and brittle for static checking.🛠️ Proposed fix
-def _gather_ocp_storage_class() -> tuple[dict[str, str], str, str]: +def _gather_ocp_storage_class() -> tuple[dict[str, Any], str, str]: @@ - job_creds: dict[str, str] = {"host": host, "username": username, "password": password} + job_creds: dict[str, Any] = {"host": host, "username": username, "password": password}#!/bin/bash python - <<'PY' from pathlib import Path text = Path("cli/mtv_api_tests/generate.py").read_text() checks = [ "def _gather_ocp_storage_class() -> tuple[dict[str, str], str, str]:", 'job_creds: dict[str, str] = {"host": host, "username": username, "password": password}', 'job_creds["verify_ssl"] = ocp_creds["verify_ssl"]', 'job_creds["ca_bundle"] = ocp_creds["ca_bundle"]', ] for check in checks: print(("FOUND" if check in text else "MISSING"), check) PYAs per coding guidelines, "Variables must have consistent types. Use type annotations and default to empty containers (e.g., {}, []) to avoid None checks." and "All new functions and functions with signature changes must have complete type annotations using built-in Python typing (dict, list, tuple)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/generate.py` around lines 199 - 260, The function _gather_ocp_storage_class currently declares a return type of tuple[dict[str, str], str, str] and creates job_creds: dict[str, str], but it can include non-string fields ("verify_ssl", "ca_bundle") and may return the original heterogeneous ocp_creds; change the type annotations to reflect heterogeneous values (e.g., use dict[str, Any] or Mapping[str, Any]) for the job_creds variable and the function return, update the job_creds annotation and the function signature accordingly, and add/adjust the typing import (Any or Mapping) so static type checkers accept assignments from ocp_creds and the added TLS fields; keep the logic in _gather_ocp_storage_class, referencing job_creds, ocp_creds, and get_ocp_credentials.cli/mtv_api_tests/common.py (3)
782-795:⚠️ Potential issue | 🟠 MajorHIGH: Reject non-object
.providers.jsonroots before mutating them.
json.loads()succeeding is not enough here. If the file contains valid JSON like[]or"foo",existing[provider_key] = provider_configraisesTypeErrorand the CLI aborts. Treat any non-dict root as invalid, warn, and recover with{}.🔧 Minimal fix
if providers_path.exists(): try: - existing = json.loads(providers_path.read_text()) + loaded = json.loads(providers_path.read_text()) + if isinstance(loaded, dict): + existing = loaded + else: + console.print( + f" [yellow]Warning: existing {providers_path} must contain a JSON object, will overwrite[/yellow]" + ) + existing = {} except json.JSONDecodeError: console.print(f" [yellow]Warning: existing {providers_path} is invalid, will overwrite[/yellow]")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/common.py` around lines 782 - 795, When loading providers_path into existing, ensure the parsed JSON is a dict before mutating: after json.loads(...) in the existing assignment block validate that existing is an instance of dict, and if not (including when JSONDecodeError was caught) print the same warning and set existing = {} so subsequent existing[provider_key] = provider_config won't raise TypeError; keep using Confirm.ask for overwrite prompts and then call _write_secret_file(providers_path, json.dumps(existing, indent=2) + "\n") as before.
401-419:⚠️ Potential issue | 🟠 MajorHIGH: This regresses the vSphere TLS hardening.
ssl_config=Nonecurrently becomesverify_ssl=False, andssl.create_default_context(cafile=...)still executes before thetry. That means a caller that forgets to forward TLS settings silently disables cert validation, while unreadable or malformed CA files bypass the normalizedConnectionErrorflow.🔧 Minimal fix
- verify_ssl = ssl_config.get("verify_ssl", "false") == "true" + verify_ssl = ssl_config.get("verify_ssl", "true") == "true" ca_bundle = ssl_config.get("ca_bundle", "") connect_kwargs: dict[str, Any] = {"host": host, "user": username, "pwd": password, "port": 443} - if verify_ssl and ca_bundle: - ssl_context = ssl.create_default_context(cafile=ca_bundle) - connect_kwargs["sslContext"] = ssl_context - elif not verify_ssl: - console.print(" [dim]SSL certificate validation disabled for vSphere connection[/dim]") - connect_kwargs["disableSslCertValidation"] = True with console.status("[bold]Connecting to vSphere..."): try: + if verify_ssl and ca_bundle: + connect_kwargs["sslContext"] = ssl.create_default_context(cafile=ca_bundle) + elif not verify_ssl: + console.print(" [dim]SSL certificate validation disabled for vSphere connection[/dim]") + connect_kwargs["disableSslCertValidation"] = True si = SmartConnect(**connect_kwargs) + except OSError as exc: + raise ConnectionError(f"Failed to load vSphere CA bundle '{ca_bundle}': {exc}") from exc except vim.fault.InvalidLogin as exc: raise ConnectionError(f"Invalid vSphere credentials for {host}") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/common.py` around lines 401 - 419, The code currently treats ssl_config=None as disabling verification and calls ssl.create_default_context outside the try, which can raise and bypass the ConnectionError flow; change the logic so that a None ssl_config defaults to strict verification (treat None as verify_ssl=True), move creation of ssl_context (ssl.create_default_context(cafile=ca_bundle)) into the try block immediately before SmartConnect, and wrap that creation so any exceptions are caught and re-raised as ConnectionError; update references ssl_config, verify_ssl, ca_bundle, connect_kwargs and SmartConnect accordingly and only set connect_kwargs["sslContext"] when ssl_context was successfully created.
649-692:⚠️ Potential issue | 🟠 MajorHIGH: The
feat: Add test configuration and execution toolhandoff still drops vSphere TLS state.
get_vsphere_credentials()collectsverify_ssl/ca_bundle, butbuild_providers_json()never serializes them. Discovery can succeed with non-default vSphere trust and the saved provider then loses that state for later local/job execution. Persist the TLS fields here, or fail fast duringgeneratewhen the selected trust mode is not replayable.🔧 Minimal direction
def build_providers_json( vsphere_host: str, vsphere_user: str, vsphere_pass: str, version: str, guest_linux_user: str, guest_linux_pass: str, copyoffload_config: dict[str, Any], + ssl_config: dict[str, str], guest_vm_win_user: str = "", guest_vm_win_password: str = "", ) -> tuple[str, dict[str, Any]]: @@ provider_config: dict[str, Any] = { "type": "vsphere", "version": version, "fqdn": vsphere_host, "api_url": f"https://{vsphere_host}/sdk", "username": vsphere_user, "password": vsphere_pass, "guest_vm_linux_user": guest_linux_user, "guest_vm_linux_password": guest_linux_pass, "copyoffload": copyoffload_config, } + if "verify_ssl" in ssl_config: + provider_config["verify_ssl"] = ssl_config["verify_ssl"] + if "ca_bundle" in ssl_config: + provider_config["ca_bundle"] = ssl_config["ca_bundle"]Based on learnings:
generateandrunare intentionally separate CLI subcommands.run --mode jobexpects the Job YAML manifest to already exist (created by a priorgenerateinvocation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/common.py` around lines 649 - 692, build_providers_json currently omits the vSphere TLS state collected earlier, so update build_providers_json to include the TLS fields (the verify_ssl boolean and ca_bundle path/content) into provider_config (use the same keys produced by get_vsphere_credentials, e.g. "verify_ssl" and "ca_bundle" or the existing canonical keys used elsewhere) so the generated .providers.json preserves trust mode; if those fields are absent or indicate a non-replayable trust mode, either persist explicit values or raise a clear error in build_providers_json (or in generate) to fail-fast when the selected trust mode cannot be replayed later by run --mode 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 `@cli/mtv_api_tests/generate.py`:
- Around line 359-375: Update the generated shell commands to include the
documented "uv run " prefix and safely quote variable interpolations: prepend
"uv run " to the local command assembly stored in local_cmd (where vsphere_key,
ocp_key, storage_class, resolved_category and test_filter are used) and wrap
vsphere_key, ocp_key, storage_class, resolved_category, namespace and job_name
with shlex.quote() when interpolating; also change the hardcoded job invocation
in the Panel (currently "mtv-api-tests run --mode job") to "uv run mtv-api-tests
run --mode job" and ensure the oc logs command uses shlex.quote(namespace) and
shlex.quote(job_name) when building "oc logs -f -n ... job/...".
In `@cli/mtv_api_tests/run.py`:
- Around line 128-146: The else after the raise is redundant; remove the else
and dedent the block that creates tc_args, reads verify_ssl, builds env_vars and
returns them so the code after the raise executes in the normal path;
specifically update the logic around ocp_provider to keep the console.print, the
tc_args initialization, handling of verify_ssl (verify_ssl_bool and the
insecure_verify_skip tc arg), the env_vars dict construction
(CLUSTER_USERNAME/CLUSTER_PASSWORD) and the final return tc_args, env_vars at
the top level of the if-not-all check instead of inside an else.
In `@Dockerfile`:
- Around line 42-46: The Dockerfile runs pre-compilation via "uv sync"
(UV_COMPILE_BYTECODE=1) but then removes compiled bytecode with the "find
${APP_DIR} -type d -name \"__pycache__\" ... rm -rfv" line; either stop deleting
those __pycache__ directories or disable bytecode compilation. To fix, decide
which behavior you want and implement it: (A) preserve precompiled files by
removing the "find ${APP_DIR} -type d -name \"__pycache__\" -print0 | xargs -0
-r rm -rfv" removal (and keep UV_COMPILE_BYTECODE=1 for "uv sync"), or (B)
disable compilation by ensuring UV_COMPILE_BYTECODE=0 (or unset) when running
"uv sync" so the deletion becomes unnecessary; update the Dockerfile around the
"uv sync" invocation and the cleanup step accordingly and keep references to
UV_COMPILE_BYTECODE, uv sync, and the find __pycache__ command consistent.
In `@README.md`:
- Around line 501-519: The README incorrectly states "All interactive prompts
can be bypassed" while cli/mtv_api_tests/generate.py still prompts for
discovered resources and vendor-specific inputs; either update the README
examples and wording for "uv run mtv-api-tests generate" to clearly state that
generate currently requires interactive inputs for discovered resources
(datastores, VM/ESXi host, storage class, storage vendor, guest/storage
credentials) or implement CLI flags in generate.py to accept those values
non-interactively (e.g., add flags for datastore, vm, esxi_host, storage_class,
storage_vendor, guest_creds, storage_creds) and then document those flags in
README so CI usage is actually non-interactive.
---
Duplicate comments:
In `@cli/mtv_api_tests/common.py`:
- Around line 782-795: When loading providers_path into existing, ensure the
parsed JSON is a dict before mutating: after json.loads(...) in the existing
assignment block validate that existing is an instance of dict, and if not
(including when JSONDecodeError was caught) print the same warning and set
existing = {} so subsequent existing[provider_key] = provider_config won't raise
TypeError; keep using Confirm.ask for overwrite prompts and then call
_write_secret_file(providers_path, json.dumps(existing, indent=2) + "\n") as
before.
- Around line 401-419: The code currently treats ssl_config=None as disabling
verification and calls ssl.create_default_context outside the try, which can
raise and bypass the ConnectionError flow; change the logic so that a None
ssl_config defaults to strict verification (treat None as verify_ssl=True), move
creation of ssl_context (ssl.create_default_context(cafile=ca_bundle)) into the
try block immediately before SmartConnect, and wrap that creation so any
exceptions are caught and re-raised as ConnectionError; update references
ssl_config, verify_ssl, ca_bundle, connect_kwargs and SmartConnect accordingly
and only set connect_kwargs["sslContext"] when ssl_context was successfully
created.
- Around line 649-692: build_providers_json currently omits the vSphere TLS
state collected earlier, so update build_providers_json to include the TLS
fields (the verify_ssl boolean and ca_bundle path/content) into provider_config
(use the same keys produced by get_vsphere_credentials, e.g. "verify_ssl" and
"ca_bundle" or the existing canonical keys used elsewhere) so the generated
.providers.json preserves trust mode; if those fields are absent or indicate a
non-replayable trust mode, either persist explicit values or raise a clear error
in build_providers_json (or in generate) to fail-fast when the selected trust
mode cannot be replayed later by run --mode job.
In `@cli/mtv_api_tests/generate.py`:
- Around line 199-260: The function _gather_ocp_storage_class currently declares
a return type of tuple[dict[str, str], str, str] and creates job_creds:
dict[str, str], but it can include non-string fields ("verify_ssl", "ca_bundle")
and may return the original heterogeneous ocp_creds; change the type annotations
to reflect heterogeneous values (e.g., use dict[str, Any] or Mapping[str, Any])
for the job_creds variable and the function return, update the job_creds
annotation and the function signature accordingly, and add/adjust the typing
import (Any or Mapping) so static type checkers accept assignments from
ocp_creds and the added TLS fields; keep the logic in _gather_ocp_storage_class,
referencing job_creds, ocp_creds, and get_ocp_credentials.
In `@utilities/utils.py`:
- Around line 633-635: The CLUSTER_VERIFY_SSL env var parsing is too
permissive—normalize its value by calling strip().lower() on verify_ssl_env
before checking membership so values like "true ", " YES\n" are recognized;
update the logic that sets insecure_verify_skip (using the verify_ssl_env
variable) to use the normalized string and keep the same truthy set
("true","1","yes") while preserving the current inverse assignment
(insecure_verify_skip = normalized not in (...)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bd02645-2d5f-4dac-be0c-c2ee08e07201
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.dockerignore.gitignoreDockerfileREADME.mdcli/__init__.pycli/mtv_api_tests/__init__.pycli/mtv_api_tests/__main__.pycli/mtv_api_tests/common.pycli/mtv_api_tests/generate.pycli/mtv_api_tests/run.pyguides/copyoffload/how-to-run-copyoffload-tests.mdpyproject.tomlutilities/utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
utilities/utils.py (1)
633-635:⚠️ Potential issue | 🟠 MajorHIGH: Normalize
CLUSTER_VERIFY_SSLwithstrip()before evaluating.
verify_ssl_env.lower()misclassifies values like"true "/"yes\n"and silently flips TLS verification off. Keep lenient parsing, but normalize first.🔧 Proposed fix
verify_ssl_env = os.environ.get("CLUSTER_VERIFY_SSL") if verify_ssl_env is not None: - insecure_verify_skip = verify_ssl_env.lower() not in ("true", "1", "yes") + insecure_verify_skip = verify_ssl_env.strip().lower() not in ("true", "1", "yes")Based on learnings,
CLUSTER_VERIFY_SSLparsing is intentionally lenient but expected to be case-insensitive after strip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/utils.py` around lines 633 - 635, The CLUSTER_VERIFY_SSL environment value is not normalized before checking, so update the logic that reads verify_ssl_env (from os.environ.get("CLUSTER_VERIFY_SSL")) to call strip() before lowercasing and evaluating; specifically, normalize verify_ssl_env = verify_ssl_env.strip() (or combine .strip().lower()) then test membership against ("true","1","yes") when setting insecure_verify_skip so values like "true " or "yes\n" are handled correctly.cli/mtv_api_tests/common.py (1)
649-692:⚠️ Potential issue | 🟠 MajorHIGH: The generated vSphere provider drops the TLS settings collected during
generate.For the
feat: Add test configuration and execution toolpath,.providers.jsonis the replay artifact.get_vsphere_credentials()collectsverify_ssl/ca_bundle, andconnect_vsphere()uses them for discovery, butbuild_providers_json()serializes neither. A custom-CA vCenter orverify_ssl=falsesetup can therefore work duringgenerateand then reconnect with different trust settings later.Possible fix
def build_providers_json( vsphere_host: str, vsphere_user: str, vsphere_pass: str, version: str, guest_linux_user: str, guest_linux_pass: str, copyoffload_config: dict[str, Any], + ssl_config: dict[str, str], guest_vm_win_user: str = "", guest_vm_win_password: str = "", ) -> tuple[str, dict[str, Any]]: @@ provider_config: dict[str, Any] = { "type": "vsphere", "version": version, "fqdn": vsphere_host, "api_url": f"https://{vsphere_host}/sdk", "username": vsphere_user, "password": vsphere_pass, "guest_vm_linux_user": guest_linux_user, "guest_vm_linux_password": guest_linux_pass, "copyoffload": copyoffload_config, } + if "verify_ssl" in ssl_config: + provider_config["verify_ssl"] = ssl_config["verify_ssl"] + if "ca_bundle" in ssl_config: + provider_config["ca_bundle"] = ssl_config["ca_bundle"]Based on learnings:
generateandrunare intentionally separate CLI subcommands, andrun --mode jobexpects artifacts created by a priorgenerateinvocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/common.py` around lines 649 - 692, build_providers_json currently omits the vSphere TLS trust settings, causing artifacts from generate to lose verify_ssl/ca_bundle; update the build_providers_json signature to accept optional verify_ssl (bool | None) and ca_bundle (str | None) parameters and, inside the function, add those keys to provider_config (e.g., provider_config["verify_ssl"] = verify_ssl and provider_config["ca_bundle"] = ca_bundle) only when they are not None so the serialized provider entry preserves the TLS settings used during generate; reference build_providers_json and provider_config when making this change.cli/mtv_api_tests/run.py (1)
240-248:⚠️ Potential issue | 🟠 MajorHIGH: Only treat
job/... unchangedas a stale rerun.This applies Namespace, Secret, and Job together. The current substring check bails out on any
unchanged, so a valid result likenamespace/... unchangedplusjob.batch/... createdstill exits1after the Job was actually created.🛠️ Narrow the check to Job status lines
- combined_output = (result.stdout or "") + (result.stderr or "") - if "unchanged" in combined_output: + combined_output = (result.stdout or "") + "\n" + (result.stderr or "") + job_status_lines = [ + line.strip().lower() + for line in combined_output.splitlines() + if line.lstrip().startswith(("job/", "job.batch/")) + ] + if any(line.endswith(" unchanged") for line in job_status_lines): console.print( "[yellow]Warning: Job already exists and was not modified. " "A completed Job will not re-run.[/yellow]\n" f" [dim]Delete first:[/dim] oc delete -f {job_yaml_path}" )Can `oc apply -f` on a multi-document manifest report `namespace/... unchanged` and `job.batch/... created` in the same invocation, and does reapplying an existing Kubernetes Job leave the Job unchanged instead of starting a new run?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/run.py` around lines 240 - 248, The current check inspects combined_output for the substring "unchanged" and exits even if that "unchanged" refers to Namespace or Secret; update the logic around combined_output (used where job_yaml_path is referenced) to only treat a stale rerun when a Job line reports unchanged — e.g., split combined_output into lines and use a regex or string match to detect lines containing a Job resource (match "job." or "job.batch/" or lines beginning with "job" plus " unchanged" on the same line) and only call console.print and raise typer.Exit(code=1) when such a Job-specific unchanged line is found. Ensure other "unchanged" lines do not trigger the exit.cli/mtv_api_tests/generate.py (2)
245-256:⚠️ Potential issue | 🟠 MajorHIGH: the kubeconfig flow still drops TLS settings from discovery.
Here
job_credsis rebuilt from prompt values, but the source dict in the kubeconfig branch intentionally contains onlykubeconfig. That makes theverify_ssl/ca_bundlecopies on Lines 253-256 effectively dead code for the path that needs them most, so the generated provider and Job can stop matching the cluster TLS settings that discovery just used. Derive these values fromocp_client.configurationor prompt for them in this branch. Forfeat: Add test configuration and execution tool, this is a functional gap on self-signed/custom-CA clusters.Based on learnings,
get_ocp_credentials()intentionally returns only{"kubeconfig": kubeconfig}whenKUBECONFIGis set, so callers that need explicit TLS fields must derive or prompt them locally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/generate.py` around lines 245 - 256, The kubeconfig branch rebuilds job_creds from prompts but never pulls TLS settings, so verify_ssl/ca_bundle are missing for KUBECONFIG flows; update the branch that creates job_creds (the block using Prompt.ask and variable job_creds) to populate verify_ssl and ca_bundle by reading ocp_client.configuration (e.g., ocp_client.configuration.verify_ssl and ocp_client.configuration.ssl_ca_cert or equivalent) or, if those aren’t available, prompt the user for those values and then add them into job_creds so the generated provider/Job preserves the cluster TLS settings used by discovery.
58-62:⚠️ Potential issue | 🟠 MajorHIGH: cleanup can still replace the real wizard failure.
If the
with _vsphere_session(...)body raises anddisconnect_vsphere(si)also raises here, Python will surface the disconnect error and discard the original discovery/selection failure. Handle disconnect errors separately during unwind—log/suppress them only when another exception is already active, and re-raise them on the clean path.#!/bin/bash python - <<'PY' from contextlib import contextmanager `@contextmanager` def cm(): try: yield finally: raise RuntimeError("cleanup") try: with cm(): raise ValueError("body") except Exception as exc: print(type(exc).__name__, exc) PY rg -n -A12 -B4 '^def disconnect_vsphere\b' cli/mtv_api_tests/common.pyExpected result: the Python snippet prints
RuntimeError cleanup, showing that a cleanup exception replaces the body exception; ifdisconnect_vsphere()has no internal guard, the same masking risk exists here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/mtv_api_tests/generate.py` around lines 58 - 62, The finally block in the _vsphere_session context manager currently calls disconnect_vsphere(si) directly which can mask an exception from the with-body if disconnect_vsphere also raises; wrap the disconnect in a try/except inside the finally, detect whether an exception is already active (via sys.exc_info() or sys.exc_info()[0]), and if there is an active exception log/suppress the disconnect error (use the module logger or logging.exception) but do not re-raise it; if there is no active exception, re-raise the disconnect error so cleanup failures surface normally — apply this change around the disconnect_vsphere(si) call that follows connect_vsphere(host, user, password, ssl_config=ssl_config) and yield si.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/mtv_api_tests/common.py`:
- Around line 405-419: Exceptions from creating ssl_context (when verify_ssl and
ca_bundle are set) occur outside the try/except, bypassing the ConnectionError
handling; move the CA bundle loading and ssl_context creation into the same
guarded block that calls SmartConnect so any PEM/read errors are caught and
wrapped. Specifically, inside the block that uses console.status and the
try/except around SmartConnect, attempt to create ssl_context (using
ssl.create_default_context with cafile=ca_bundle) and set
connect_kwargs["sslContext"]=ssl_context there, keeping the existing branch that
sets connect_kwargs["disableSslCertValidation"]=True when not verify_ssl; ensure
you still catch vim.fault.InvalidLogin and general Exception and raise
ConnectionError with the host context.
- Around line 559-579: The code incorrectly treats kubeconfig-only auth as
insecure because verify_ssl is derived from creds.get before branching; move the
verify_ssl logic so it only applies when the non-kubeconfig branch is taken:
compute verify_ssl and ca_bundle and call
urllib3.disable_warnings/InsecureRequestWarning and console.print only inside
the else branch where get_client(...) is called with host/username/password, and
set KUBERNETES_CLIENT_CA_BUNDLE only in that same branch when verify_ssl and
ca_bundle are truthy; keep the kubeconfig branch (the "kubeconfig" in creds
path) untouched so it does not suppress warnings or print the insecure message.
In `@cli/mtv_api_tests/generate.py`:
- Around line 111-114: The wizard currently allows selecting any VM via
discover_vms() / select_from_list() but only collects and persists Linux guest
credentials (see selected_vm["guest_os"], credential-collection block at lines
164-166 and persistence at 305-313), so a Windows VM selection will produce an
unusable config; either restrict the choices in select_from_list() to Linux
guests (filter discover_vms() results where guest_os indicates Linux) or add
branching after selection on selected_vm["guest_os"] to (a) prompt for
Windows-specific credentials when guest_os is Windows, (b) persist the
corresponding auth fields in the same code path that now writes Linux auth, and
(c) if an unsupported OS is selected, fail fast with a clear error instead of
accepting it.
In `@cli/mtv_api_tests/run.py`:
- Around line 110-123: The docstring for _get_cluster_config is missing a Raises
section; update its docstring to include a Raises: entry that documents that
typer.Exit is raised when provider credentials are incomplete (e.g., missing
username/password) so callers and readers know this behavior; ensure the Raises
section uses the same docstring style as Args/Returns and mentions typer.Exit
and the condition under which it is raised.
- Around line 138-145: The provider-selected TLS choice is only passed via the
test-case flag (--tc=insecure_verify_skip) while the preserved environment can
still contain CLUSTER_VERIFY_SSL which utilities/utils.py:get_cluster_client()
reads first; update the local run environment in _run_local() (the env_vars dict
built alongside tc_args in _get_cluster_config()/run.py) to explicitly set
CLUSTER_VERIFY_SSL to the provider-derived value (or remove/unset it) based on
the computed verify_ssl_bool so the selected provider cannot be silently
overridden by ambient env.
---
Duplicate comments:
In `@cli/mtv_api_tests/common.py`:
- Around line 649-692: build_providers_json currently omits the vSphere TLS
trust settings, causing artifacts from generate to lose verify_ssl/ca_bundle;
update the build_providers_json signature to accept optional verify_ssl (bool |
None) and ca_bundle (str | None) parameters and, inside the function, add those
keys to provider_config (e.g., provider_config["verify_ssl"] = verify_ssl and
provider_config["ca_bundle"] = ca_bundle) only when they are not None so the
serialized provider entry preserves the TLS settings used during generate;
reference build_providers_json and provider_config when making this change.
In `@cli/mtv_api_tests/generate.py`:
- Around line 245-256: The kubeconfig branch rebuilds job_creds from prompts but
never pulls TLS settings, so verify_ssl/ca_bundle are missing for KUBECONFIG
flows; update the branch that creates job_creds (the block using Prompt.ask and
variable job_creds) to populate verify_ssl and ca_bundle by reading
ocp_client.configuration (e.g., ocp_client.configuration.verify_ssl and
ocp_client.configuration.ssl_ca_cert or equivalent) or, if those aren’t
available, prompt the user for those values and then add them into job_creds so
the generated provider/Job preserves the cluster TLS settings used by discovery.
- Around line 58-62: The finally block in the _vsphere_session context manager
currently calls disconnect_vsphere(si) directly which can mask an exception from
the with-body if disconnect_vsphere also raises; wrap the disconnect in a
try/except inside the finally, detect whether an exception is already active
(via sys.exc_info() or sys.exc_info()[0]), and if there is an active exception
log/suppress the disconnect error (use the module logger or logging.exception)
but do not re-raise it; if there is no active exception, re-raise the disconnect
error so cleanup failures surface normally — apply this change around the
disconnect_vsphere(si) call that follows connect_vsphere(host, user, password,
ssl_config=ssl_config) and yield si.
In `@cli/mtv_api_tests/run.py`:
- Around line 240-248: The current check inspects combined_output for the
substring "unchanged" and exits even if that "unchanged" refers to Namespace or
Secret; update the logic around combined_output (used where job_yaml_path is
referenced) to only treat a stale rerun when a Job line reports unchanged —
e.g., split combined_output into lines and use a regex or string match to detect
lines containing a Job resource (match "job." or "job.batch/" or lines beginning
with "job" plus " unchanged" on the same line) and only call console.print and
raise typer.Exit(code=1) when such a Job-specific unchanged line is found.
Ensure other "unchanged" lines do not trigger the exit.
In `@utilities/utils.py`:
- Around line 633-635: The CLUSTER_VERIFY_SSL environment value is not
normalized before checking, so update the logic that reads verify_ssl_env (from
os.environ.get("CLUSTER_VERIFY_SSL")) to call strip() before lowercasing and
evaluating; specifically, normalize verify_ssl_env = verify_ssl_env.strip() (or
combine .strip().lower()) then test membership against ("true","1","yes") when
setting insecure_verify_skip so values like "true " or "yes\n" are handled
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48ad26f2-ad3f-4fd1-bc70-453d709f0b42
📒 Files selected for processing (4)
cli/mtv_api_tests/common.pycli/mtv_api_tests/generate.pycli/mtv_api_tests/run.pyutilities/utils.py
Contributor
Author
|
@coderabbitai[bot] The following review comments were reviewed and a decision was made:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
myakove
requested changes
Mar 26, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/mtv_api_tests/common.py`:
- Around line 269-308: The _get_ssl_config function currently raises typer.Exit
when a provided CA bundle path does not exist; update its docstring to include a
Raises: section documenting that typer.Exit is raised on an invalid/nonexistent
CA bundle (include the condition: when verify_ssl is true and the specified CA
bundle path is not found), keeping the existing Args and Returns sections intact
so callers and tests can rely on the explicit exit behavior.
- Around line 372-376: The explicit-credentials branch currently reads
CLUSTER_HOST/USERNAME/PASSWORD but then calls
_get_ssl_config("MTV_API_TESTS_CLUSTER"), which ignores CLUSTER_VERIFY_SSL and
CLUSTER_CA_BUNDLE; update the call in that branch to use the CLUSTER prefix
(call _get_ssl_config("CLUSTER")) or merge both prefixes so TLS settings from
CLUSTER_* are honored when host/username/password are supplied; modify the
return construction around host/username/password and _get_ssl_config to ensure
the resulting dict includes the CA/verify values from CLUSTER_VERIFY_SSL and
CLUSTER_CA_BUNDLE.
- Around line 721-729: The provider_config being built in common.py currently
emits "host" and "storage_class" and omits "fqdn", which breaks the documented
provider shape; update the provider_config dict (the variable named
provider_config) to include "fqdn" (set to the cluster hostname value), ensure
"api_url" remains present (pointing to the cluster endpoint), and remove the
"host" and "storage_class" keys so the record matches .providers.json.example
while keeping "type", "version", "username", and "password" intact.
- Around line 401-403: The code currently treats a missing verify_ssl key as
"false" which weakens TLS; update the logic that reads ssl_config (variable
ssl_config and derived verify_ssl/ca_bundle) to treat an omitted verify_ssl as
enabled (true) — e.g. only set verify_ssl to false when the key is present and
explicitly set to "false", otherwise default to true; apply the same fix in the
other occurrences that read ssl_config (the callsites used by connect_vsphere
and the job-generation code that emits cluster_verify_ssl and
--tc=insecure_verify_skip) so missing keys no longer disable certificate
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6deb9858-bdc3-4a22-ade9-403f993e1da5
📒 Files selected for processing (3)
cli/mtv_api_tests/common.pypyproject.tomlutilities/utils.py
5 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/mtv_api_tests/common.py`:
- Around line 1027-1069: The generated manifest currently creates a unique
namespace and Secret (namespace, secret_name) but never deletes them; update the
YAML assembly where yaml_content is built (and/or the caller that uses
_generate_namespace_name/_build_job_spec) to append an explicit cleanup resource
that removes the namespace and Secret after the test Job completes—for example
add a one-shot Kubernetes Job (e.g., named f"{job_name}-cleanup") that runs a
small container to delete the namespace (or calls the k8s API) once the main Job
finishes, or set appropriate ownerReferences/cleanup logic so the namespace and
Secret are removed; ensure you do not rely on ttlSecondsAfterFinished alone
since it won't remove the namespace/Secret.
- Around line 569-584: The code sets the process-wide env var
KUBERNETES_CLIENT_CA_BUNDLE before calling get_client(), but never restores it,
causing subsequent connect_ocp() calls to inherit the wrong CA bundle; modify
connect_ocp() to save the previous os.environ.get("KUBERNETES_CLIENT_CA_BUNDLE")
value, set the new ca_bundle only for the duration of the get_client(...) call,
and restore the original value in a finally block (clearing the key if it was
previously absent) so that get_client() uses the custom bundle without leaking
it to later calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a689de11-f10c-46f8-bd35-fe3201513a9d
📒 Files selected for processing (1)
cli/mtv_api_tests/common.py
Introduce a cli tool mtv-api-tests that automates configuration and test execution. It discovers vSphere and OpenShift resources via their APIs, generates `.providers.json` and a self-contained `mtv-api-tests-manifests.yaml` (Namespace + Secret + Job), and can run tests locally or as an OpenShift Job. Generate Configuration ```bash uv run mtv-api-tests generate ``` The wizard will: 1. Prompt for vSphere credentials (or read from `VSPHERE_HOST`, `VSPHERE_USERNAME`, `VSPHERE_PASSWORD`) 2. Connect to vSphere and let you select datastores, a test VM, and an ESXi host 3. Prompt for storage vendor and credentials 4. Connect to OpenShift and let you select a storage class 5. Ask which test category to run (`all`, `copyoffload`, `tier0`, `warm`, `remote`) 6. Write `.providers.json` and `mtv-api-tests-manifests.yaml` Run Tests ```bash uv run mtv-api-tests run --mode local uv run mtv-api-tests run --mode job ``` Signed-off-by: Roy Golan <rgolan@redhat.com> fix: address PR review feedback - security, validation, and config improvements - Atomic file writes in _write_secret_file - Quote shell variables in Job manifest and Dockerfile - Validate ocp_creds and provider entries early - Store CA bundle PEM contents instead of path - Add env var fallbacks for cluster credentials - Fail on incomplete OCP provider instead of silent fallback - Pin uv version in Dockerfile - Add .env to .dockerignore - Fix .gitignore .claude/ negation patterns
- Replace os.write() with os.fdopen() in _write_secret_file() to prevent partial writes - Split generate_job_yaml() into focused helpers - Add --tc=cluster_host and --tc=insecure_verify_skip to Job pytest command - Chain typer.Exit exceptions with 'from exc' in generate.py - Fix CLUSTER_VERIFY_SSL env var priority over py_config default - Change .claude/ to .claude/* in .gitignore for proper allowlist - Use neutral wording for unchanged provider messages - Only set insecure_verify_skip when verify_ssl is explicitly configured - Multi-stage Dockerfile to remove build-only packages from runtime image
for more information, see https://pre-commit.ci
get_cluster_client() only treated the exact string "true" as enabling SSL verification. Now accepts "true", "1", and "yes" (case-insensitive), matching the CLI's parsing logic.
- Ensure parent directory exists in _write_secret_file() - Replace manual try/finally with context manager for vSphere session - Show fully reproducible command in Next Steps panel - Detect unchanged Job on oc apply and fail with guidance - Move .providers.json validation before user prompts - Replace isinstance(DynamicClient) with None check
- Revert client check to isinstance(client, DynamicClient) for FakeClient support - Switch build backend from setuptools to hatchling - Revert CLUSTER_* env var prefix change (out of scope for this PR)
- Default verify_ssl to "true" instead of "false" (secure-by-default) - Fix leftover MTV_API_TESTS_CLUSTER prefix to CLUSTER - Add Raises section to _get_ssl_config() docstring
Contributor
Author
|
/verified |
Collaborator
|
/approve |
myakove
approved these changes
Mar 30, 2026
Collaborator
|
@rgolangh |
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
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
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.
mtv-api-tests.py automates configuration and test execution.
It discovers vSphere and OpenShift resources via their APIs,
generates
.providers.jsonand a self-containedmtv-api-tests-manifest.yaml(Namespace + Secret + Job), and can run tests locally or as an OpenShift Job.
Generate Configuration
The wizard will:
VSPHERE_HOST,VSPHERE_USERNAME,VSPHERE_PASSWORD)all,copyoffload,tier0,warm,remote).providers.jsonandmtv-api-tests-manifest.yamlRun Tests
Signed-off-by: Roy Golan rgolan@redhat.com
Summary by CodeRabbit
New Features
Documentation
Chores
Ignore rules