Repository navigation
OSAC-1221: add InstanceType E2E tests - #104
Conversation
|
@ygalblum: This pull request references OSAC-1221 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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 private gRPC and CLI instance-type helpers, extends compute-instance creation to accept instance types, and adds end-to-end tests for instance-type lifecycle plus compute-instance success and failure cases. ChangesInstance type test support
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/osac_cli.py`:
- Around line 92-96: The create_instance_type helper is returning raw CLI stdout
instead of the UUID implied by its API, so align it with create_compute_instance
and create_cluster by parsing the UUID from the command output before returning
it. Update create_instance_type to use the same UUID extraction approach as the
sibling helpers, or if it should not return anything, change the signature to
None and remove the return value so the behavior matches the implementation.
In `@tests/vmaas/test_compute_instance_instance_type.py`:
- Around line 113-133: The teardown in the test cleanup path is swallowing
failures by catching Exception and only printing warnings, which hides
leaked-resource regressions. Update the cleanup in
test_compute_instance_instance_type to let failures surface to the test
framework, or explicitly fail the test after logging, and apply the same change
to the other cleanup block at the referenced duplicate location. Keep the
cleanup logic around cli.delete_compute_instance, wait_for_deletion, and
private_grpc.delete_instance_type, but do not silently continue on exceptions.
- Around line 141-165: The negative-path checks around run_unchecked for create
computeinstance can leave real resources behind if the command unexpectedly
succeeds. Update the relevant test cases in test_compute_instance_instance_type
to capture the created instance UUID or use a try/finally-style cleanup path
before asserting failure, so any successful create is deleted even when the test
later fails. Make the same fix for the other affected create computeinstance
block in the same file, and ensure the cleanup runs before delete_instance_type
is attempted.
- Around line 108-111: The deprecated create flow in
test_compute_instance_instance_type.py only checks dep_rc and then conditionally
parses dep_output, so the test can silently skip reconciliation and cleanup if
the UUID regex stops matching. Update the test around the deprecated create
assertion to explicitly verify a UUID was returned from the CLI output before
calling wait_for_cr or performing cleanup, using the existing dep_output,
uuid_match, and deprecated_ci_uuid variables so the test fails immediately when
the expected UUID is missing.
- Around line 141-150: The negative instance-type check in the compute instance
test uses a fixed shared value, which can make the result depend on leftovers or
parallel runs. Update the `run_unchecked` call in
`test_compute_instance_instance_type` to generate a unique missing instance-type
name the same way the nearby tests do, and keep the rest of the CLI arguments
unchanged so the failure still exercises the same path.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b6faca42-85c9-41d0-b732-8aac42ac9b55
📒 Files selected for processing (4)
tests/core/grpc_client.pytests/core/osac_cli.pytests/vmaas/test_compute_instance_instance_type.pytests/vmaas/test_instance_type_lifecycle.py
cf7af8a to
6667ee1
Compare
|
🔴 CI Triage: Root cause: PR adds E2E tests for instance-type feature but doesn't update OSAC CLI binary version from 0.0.62 to 0.0.68 Explanation: This PR (osac-test-infra#104) adds E2E tests for the InstanceType feature, including CLI methods that use the Causal chain:
Why other PRs pass: The existing test suite doesn't use instance-type features, so they work fine with the older CLI binary. This is purely a PR-specific issue where new test code depends on a newer CLI version than what's available in the container. Differential analysis: Checked 20 recent e2e-vmaas runs from the last 12 hours — other PRs (#98, #70) and periodic runs are passing. Only PR #104 is failing, confirming this is not broken_main or infra. Evidence: [ [ [ [ Suggestion: Update the OSAC_VERSION in Containerfile from 0.0.62 to 0.0.68 (or later) to pull a CLI binary that includes the instance-type feature. Specific fix: --- a/Containerfile
+++ b/Containerfile
@@ -1,7 +1,7 @@
FROM registry.access.redhat.com/ubi9/ubi:latest
ARG GRPCURL_VERSION=1.9.1
-ARG OSAC_VERSION=0.0.62
+ARG OSAC_VERSION=0.0.68Why this is the correct fix: v0.0.68 contains the CLI changes from fulfillment-service PR #735 that add:
All of these are required by the new E2E tests in this PR. Alternative consideration: The PR author could also wait for the Containerfile version to be bumped on main first, but since this PR introduces tests for a feature that's already merged and released (v0.0.68), updating the version as part of this PR is appropriate and unblocks the testing. Prow job | Build For deeper investigation, use the |
|
Waiting on osac-project/fulfillment-service#723 to merge before it can be retested |
6667ee1 to
5a9596b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Containerfile (1)
33-33: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReplace
COPY . .with explicit file copies.Copying the entire build context risks embedding unintended files. As per path instructions, copy only the specific files and directories the container needs.
- COPY . . + COPY tests/ ./tests/ + COPY pyproject.toml uv.lock ./🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Containerfile` at line 33, The Containerfile currently uses COPY . . which pulls in the entire build context and may include unintended files; replace it with explicit COPY instructions for only the required build inputs. Update the build stage near the existing COPY step to reference the specific files and directories the container needs, keeping the image contents minimal and predictable.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Containerfile`:
- Line 1: The Containerfile currently starts the final image as root; add a
non-root USER directive after all package/binary installation steps and before
WORKDIR is set. Use the existing build stages and final image setup to switch to
the intended unprivileged user so the runtime container does not execute as
root.
In `@tests/core/osac_cli.py`:
- Around line 112-125: The new instancetype helper methods in osac_cli.py are
bypassing the isolated per-instance config, so they should use the same
_run(...) path as the other CLI helpers. Update create_instance_type,
describe_instance_type, and delete_instance_type to route through _run(...) or
otherwise inject the instance’s --config arguments established in the existing
CLI wrapper, so tests using these helpers stay on the isolated authenticated
config. Keep the helper behavior aligned with the rest of the class so tests
like test_instance_type_lifecycle continue to run in parallel-worker isolation.
In `@tests/vmaas/test_compute_instance_instance_type.py`:
- Around line 167-170: The unexpected-success cleanup in the negative-path
create checks depends on parsing the CLI’s human-readable output, which can miss
cleanup if the format changes. Update the create flows in
test_compute_instance_instance_type (including the OBSOLETE-path branch) to pass
a deterministic identifier such as --name and use that identifier to delete the
ComputeInstance directly via cli.delete_compute_instance, rather than extracting
a UUID from output; keep call_unchecked() output assertions as-is, but make
cleanup independent of message formatting to avoid leaving instances behind and
racing delete_instance_type.
In `@tests/vmaas/test_instance_type_lifecycle.py`:
- Around line 87-89: The negative check after deleting the instance type is too
broad because `rc != 0` also passes for unrelated failures. Keep the existing
`run_unchecked` and `describe instancetype` assertion in
`test_instance_type_lifecycle`, but also assert the returned output matches the
not-found error path for the deleted instance type so the test proves the
resource was actually removed.
- Around line 93-96: The cleanup in the instance type lifecycle test is too
broad because cli.delete_instance_type currently catches every Exception and
masks real auth/transport/CLI failures. Update the exception handling around
delete_instance_type in test_instance_type_lifecycle to suppress only the
specific not-found/already-deleted error from the CLI and re-raise any other
exception so regressions still fail the test.
---
Outside diff comments:
In `@Containerfile`:
- Line 33: The Containerfile currently uses COPY . . which pulls in the entire
build context and may include unintended files; replace it with explicit COPY
instructions for only the required build inputs. Update the build stage near the
existing COPY step to reference the specific files and directories the container
needs, keeping the image contents minimal and predictable.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 59ecdcdc-298c-4874-8d5b-fcb462229e4b
📒 Files selected for processing (5)
Containerfiletests/core/grpc_client.pytests/core/osac_cli.pytests/vmaas/test_compute_instance_instance_type.pytests/vmaas/test_instance_type_lifecycle.py
2bc836d to
c092a72
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/grpc_client.py`:
- Around line 329-336: The update_instance_type helper currently allows
update_paths to be overridden even though it always sends only spec.state in the
object payload, which can produce mismatched masks. Fix update_instance_type by
either removing the unused update_paths parameter and hardcoding the spec.state
mask, or refactoring it to accept fields dynamically like
update_cluster_catalog_item does with **fields so the payload and updateMask
always stay in sync.
In `@tests/vmaas/test_compute_instance_instance_type.py`:
- Around line 87-105: The `run_unchecked(cli.binary, "create",
"computeinstance", ...)` argument setup is duplicated across the three compute
instance tests, making them hard to keep in sync when the CLI flags change.
Extract the shared scaffolding into a small helper (for example, a function near
these tests that builds the common `create computeinstance` args from
`vm_template`, `default_subnet`, and instance-specific values like `it_name` or
`name`), then update each test to call that helper and append any test-specific
options.
- Around line 170-172: The unexpected-success cleanup in the negative-path
create test should wait for the ComputeInstance deletion to finish before
deleting the instance type. Update the cleanup path around the
`run_unchecked(..., "delete", "computeinstance", ci_name)` call to use the same
deletion-completion handling as the happy-path flow (for example, the
`cli.delete_compute_instance` plus `wait_for_deletion` pattern used elsewhere in
`test_compute_instance_with_instance_type`). Apply the same fix in the other
affected test block so `private_grpc.delete_instance_type` only runs after the
compute instance is fully gone.
In `@tests/vmaas/test_instance_type_lifecycle.py`:
- Around line 96-101: The cleanup in the instance type lifecycle test only
checks `CalledProcessError.stderr`, but `OsacCLI._run()` may place the CLI
message in either stream. Update the teardown in `test_instance_type_lifecycle`
to inspect both `stdout` and `stderr` when deciding whether to ignore a benign
“not found” from `cli.delete_instance_type`, matching the broader handling used
by `ensure_tenant` so cleanup only re-raises unexpected errors.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3a5242e8-1b5e-453f-9e9f-a1f60b78403c
📒 Files selected for processing (4)
tests/core/grpc_client.pytests/core/osac_cli.pytests/vmaas/test_compute_instance_instance_type.pytests/vmaas/test_instance_type_lifecycle.py
c092a72 to
c9a54d5
Compare
c9a54d5 to
de13b79
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/osac_cli.py`:
- Around line 73-77: The argument-building logic in build/append path for the
CLI test helper silently ignores cores and memory_gib whenever instance_type is
provided; update the helper to detect conflicting inputs and either raise or
warn before choosing one. Use the existing instance_type branch and the
args.extend calls as the place to add validation so future callers of this
helper cannot accidentally pass both forms without noticing.
In `@tests/vmaas/test_compute_instance_instance_type.py`:
- Around line 41-145: The test_compute_instance_with_instance_type function is
too long and triggers Ruff PLR0915, so split the three clearly labeled parts
into separate tests. Move the shared instance-type creation and cleanup into a
fixture or helper, and have each new test cover one behavior: happy path
expansion, deletion protection, and deprecated warning. Keep the existing helper
symbols like private_grpc.create_instance_type, cli.create_compute_instance, and
wait_for_cr to minimize duplication and preserve isolation.
- Around line 161-172: The unexpected-success cleanup in
test_compute_instance_instance_type and
test_compute_instance_obsolete_instance_type has the delete/wait sequence
reversed. In the cleanup path after a successful create, first call wait_for_cr
to resolve the ComputeInstance CR name from the UUID, then issue
cli.delete_compute_instance(uuid=...) and finally call wait_for_deletion using
that cr_name. Keep this ordering consistent with the other cleanup blocks in
these tests so the resource is identified before deletion and the deletion wait
uses the resolved name.
- Around line 175-217: The obsolete instance type test is duplicating module
constants by hardcoding the instance type shape instead of reusing the shared
values. Update the create/update flow in
test_compute_instance_obsolete_instance_type to use IT_CORES and IT_MEMORY_GIB,
matching the happy-path test and keeping the test aligned with the module-level
constants.
- Around line 19-38: The test helper `_build_create_ci_args` is reaching into
`OsacCLI`’s private `_config_dir` instead of using a public surface. Update the
test to use a public `config_dir` property or an argument-building helper on
`OsacCLI` (for example, in `OsacCLI` itself) and keep `_build_create_ci_args`
limited to public attributes like `binary` and the new accessor.
In `@tests/vmaas/test_instance_type_lifecycle.py`:
- Line 13: The test_instance_type_lifecycle function has an unused cli fixture
parameter while only private_grpc is exercised. Remove cli from the test
signature if CLI coverage is not needed, or update the test body to use OsacCLI
by calling the new instance-type describe/get path so the fixture is actually
referenced.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b1b21268-be8b-4eaa-87a7-2937e9c88d31
📒 Files selected for processing (4)
tests/core/grpc_client.pytests/core/osac_cli.pytests/vmaas/test_compute_instance_instance_type.pytests/vmaas/test_instance_type_lifecycle.py
de13b79 to
766679f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/vmaas/test_compute_instance_instance_type.py (1)
188-199: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnexpected-success cleanup still depends on regex-parsed UUID despite a known deterministic name.
Both negative-path tests pass a deterministic
--name(ci_name) to the CLI, but the "unexpected success" cleanup branch still only actsif uuid_match:— if the CLI's quoted-UUID output format ever changes,uuid_matchisNone, theifbody is skipped, and theComputeInstancecreated despite the expected failure is never deleted, leaking a resource in the shared cluster. Sinceci_nameis already known and passed to the CLI, prefer resolving/deleting via that name directly instead of depending on output parsing succeeding.#!/bin/bash # Check whether OsacCLI/K8sClient expose a name-based delete/lookup for computeinstance rg -n -A5 'def delete_compute_instance|def get_compute_instance_name|def is_present' tests/core/osac_cli.py tests/core/k8s_client.pyAlso applies to: 223-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vmaas/test_compute_instance_instance_type.py` around lines 188 - 199, The unexpected-success cleanup in the negative-path test still depends on parsing a UUID from CLI output, so a created ComputeInstance may not be deleted if the output format changes. Update the cleanup logic in the test around the compute instance creation flow to use the already-known deterministic ci_name for lookup/deletion instead of relying on uuid_match from re.search. If the helper methods in OsacCLI or K8sClient support name-based deletion or existence checks, use those directly in the branches that currently call wait_for_cr and delete_compute_instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/osac_cli.py`:
- Around line 77-83: In osac_cli.py, the argument construction in the
instance_type fallback path uses truthy defaults that turn explicit 0 values
into 2/4, which can hide invalid-input tests. Update the args.extend logic in
the CLI helper so cores and memory_gib are only replaced when they are None,
matching the existing is not None pattern used for instance_type validation.
Keep the behavior for non-None values unchanged so explicit zeroes are preserved
and can be rejected by downstream validation.
In `@tests/vmaas/test_compute_instance_instance_type.py`:
- Around line 52-55: The fixture teardown in the instance-type cleanup block is
swallowing all failures with a bare except, which hides leaked test data in the
shared environment. Update the cleanup around private_grpc.delete_instance_type
in the fixture teardown to handle errors explicitly: catch the expected
not-found case if needed, but for any other exception log or fail the test
instead of passing silently. Keep the fix localized to the teardown helper in
test_compute_instance_instance_type so the cleanup behavior is consistent with
the other CI-safe cleanup paths in this file.
In `@tests/vmaas/test_instance_type_lifecycle.py`:
- Around line 47-66: The repeated update_instance_type, get_instance_type, and
state assertion flow in the instance type lifecycle test should be extracted
into a small helper to avoid duplication. Create a helper near the existing test
in test_instance_type_lifecycle.py that takes the target state and expected
value, then use it for the ACTIVE, DEPRECATED, and OBSOLETE transitions in
test_instance_type_lifecycle so future transitions can be added by reusing the
same logic.
---
Duplicate comments:
In `@tests/vmaas/test_compute_instance_instance_type.py`:
- Around line 188-199: The unexpected-success cleanup in the negative-path test
still depends on parsing a UUID from CLI output, so a created ComputeInstance
may not be deleted if the output format changes. Update the cleanup logic in the
test around the compute instance creation flow to use the already-known
deterministic ci_name for lookup/deletion instead of relying on uuid_match from
re.search. If the helper methods in OsacCLI or K8sClient support name-based
deletion or existence checks, use those directly in the branches that currently
call wait_for_cr and delete_compute_instance.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9685cd8b-0fbc-4a1c-a310-8c4a7454c476
📒 Files selected for processing (4)
tests/core/grpc_client.pytests/core/osac_cli.pytests/vmaas/test_compute_instance_instance_type.pytests/vmaas/test_instance_type_lifecycle.py
Add end-to-end tests for the InstanceType resource lifecycle and ComputeInstance integration with instance types. - Add InstanceType CRUD operations to GRPCClient (private API) - Add InstanceType CLI methods and instance_type parameter to OsacCLI - Add InstanceType lifecycle E2E test (create, describe, get, state transitions, delete) - Add ComputeInstance with instance_type E2E tests (happy path with reconciler expansion, deletion protection, deprecated warning, nonexistent type rejection, obsolete type rejection) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
766679f to
17b2a43
Compare
|
/retest |
|
No failed workflow runs found for this PR at commit |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omer-vishlitzky, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This branch was previously deployed
3 participants
Add end-to-end tests for the InstanceType resource lifecycle and ComputeInstance integration with instance types.
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit