Repository navigation
OSAC-1561: add Metal3 backend E2E lifecycle tests - #105
openshift-merge-bot[bot] merged 4 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
@carbonin: This pull request references [OSAC-1561](https://redhat.atlassian.net/browse/OS[AC-1](https://redhat.atlassian.net/browse/AC-1)561) 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:
WalkthroughThis PR adds BMaaS E2E support with CLI, gRPC, and Kubernetes helpers; fixtures for SSH keys and catalog items; lifecycle coverage for provisioning, power transitions, and deprovisioning; and test execution wiring. ChangesBMaaS lifecycle test support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant OsacCLI
participant GRPCClient
participant K8sClient
Test->>OsacCLI: Create BareMetalInstance
Test->>GRPCClient: Verify running state
Test->>K8sClient: Verify BMI and BMH resources
Test->>GRPCClient: Update run strategy
Test->>K8sClient: Verify powered-on state
Test->>OsacCLI: Delete BareMetalInstance
Test->>K8sClient: Verify deprovisioning
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 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 |
|
@carbonin: This pull request references OSAC-1561 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. |
f67e567 to
7d31d53
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/bmaas/test_baremetal_instance_lifecycle.py`:
- Around line 27-83: The bare metal instance lifecycle test creates a real BMI
but only deletes it at the end, so any assertion failure before teardown can
leak a BareMetalHost. Update test_baremetal_instance_lifecycle to guarantee
cleanup by wrapping the create/provision/power/deprovision flow in a try/finally
or by extracting the setup into a fixture that yields bmi_id and performs
cli.delete_baremetal_instance plus the wait helpers in teardown. Keep the
cleanup tied to the existing symbols cli.create_baremetal_instance,
cli.delete_baremetal_instance, wait_for_bmi_deletion, and
wait_for_bmi_grpc_removal so it always runs.
- Line 22: The `bmh_namespace` parameter in `test_baremetal_instance_lifecycle`
is currently passed in but never used, while `bmh_ns` is derived from
`external_host_id` instead. Either remove `bmh_namespace` from the test
signature and any callers if it is not needed, or use `bmh_namespace` in the BMH
namespace check so the test explicitly asserts the instance landed in the
expected namespace. Keep the fix localized to
`test_baremetal_instance_lifecycle` and any related assertion logic around
`bmh_ns`/BMH lookup.
In `@tests/core/helpers.py`:
- Around line 450-467: The waiters in wait_for_bmh_provisioned and
wait_for_bmh_available only poll for success states and can waste the full
timeout when the BareMetalHost has already failed. Update these helpers to check
for terminal error states on each poll, similar to wait_for_bmi_running, and
fail immediately with the current provisioning state if the host enters an error
condition. Keep the existing success checks, but add the early-fail logic around
k8s.get_bmh_provisioning_state so tests surface the real Metal3 error sooner.
In `@tests/core/osac_cli.py`:
- Around line 140-141: The osac_cli argument builder is leaking raw user_data by
appending it directly in build/compose args, so update the code path in the CLI
helper to pass user_data through a file, secret reference, or stdin-based
mechanism instead of argv. Adjust the logic around the user_data handling in the
osac_cli helper so the command line only contains a reference, not the payload
itself, and keep the existing argument assembly flow intact.
🪄 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: cd6cbda6-a976-4685-8053-554cb4893d2d
📒 Files selected for processing (10)
.gitignoreContainerfileMakefiletests/bmaas/__init__.pytests/bmaas/conftest.pytests/bmaas/test_baremetal_instance_lifecycle.pytests/core/grpc_client.pytests/core/helpers.pytests/core/k8s_client.pytests/core/osac_cli.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bmaas/test_baremetal_instance_lifecycle.py`:
- Around line 89-95: The cleanup in test_baremetal_instance_lifecycle is
re-deleting the BaremetalInstance unconditionally inside the BaseException
handler, which can raise through cli.delete_baremetal_instance and hide the
original test failure. Update the exception path to only attempt deletion if the
BMI still exists, and make the cleanup in cli.delete_baremetal_instance /
run(...) non-fatal during teardown so the original assertion error is preserved.
Keep the existing wait_for_bmi_deletion and wait_for_bmi_grpc_removal steps, but
ensure they run only after a successful cleanup attempt and do not overwrite the
initial traceback.
🪄 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: 05e6728d-cdb0-465a-ae06-9a8a016dcf44
📒 Files selected for processing (2)
tests/bmaas/test_baremetal_instance_lifecycle.pytests/core/helpers.py
be1c68e to
f286b45
Compare
f286b45 to
625353c
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 `@Containerfile`:
- Line 6: Move the package installation out of the final runtime stage in
Containerfile and convert the build to a multi-stage setup. Put the dnf install
for python3.11, python3.11-pip, make, and openssh-clients into a builder stage,
then copy only the needed runtime artifacts into the final stage. Keep the final
image free of build-time tooling by updating the relevant build stage and final
stage instructions around the existing RUN dnf install step.
In `@tests/bmaas/conftest.py`:
- Around line 57-62: The catalog-item teardown in the fixture is swallowing
delete failures by catching Exception and only printing a warning, which hides
real cleanup problems. Update the teardown path around
private_grpc.delete_baremetal_instance_catalog_item in the fixture so it still
logs the failure but re-raises the exception afterward, allowing the test to
fail when cleanup of the BareMetalInstanceCatalogItem does not succeed.
In `@tests/bmaas/test_baremetal_instance_lifecycle.py`:
- Around line 89-97: The teardown in the baremetal instance lifecycle test is
incorrectly gated on CR name lookup, so cleanup can be skipped when the CR is
not yet queryable. Update the exception handler around the BMI cleanup path to
always call cli.delete_baremetal_instance using the known bmi_id, and keep only
wait_for_bmi_deletion dependent on whether
k8s_hub_client.get_baremetal_instance_name resolved bmi_cr. Leave
wait_for_bmi_grpc_removal tied to the deletion attempt so the cleanup in
test_baremetal_instance_lifecycle remains robust even when bmi_cr is empty.
In `@tests/core/osac_cli.py`:
- Around line 17-24: The OsacCLI setup leaves behind the temporary config
directory if the initial login fails because __init__ creates _config_dir before
calling _run("login", ...), and close() is never reached on exceptions. Update
the OsacCLI initialization flow to clean up _config_dir when the login attempt
raises, using the existing _config_dir and close() cleanup path or an equivalent
try/finally around the login call so failed fixture setup does not leak
osac-config-* directories.
🪄 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: 3be24af6-f958-4d18-b0d4-a91652ded705
📒 Files selected for processing (10)
.gitignoreContainerfileMakefiletests/bmaas/__init__.pytests/bmaas/conftest.pytests/bmaas/test_baremetal_instance_lifecycle.pytests/core/grpc_client.pytests/core/helpers.pytests/core/k8s_client.pytests/core/osac_cli.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@Containerfile`:
- Line 6: Move the package installation out of the final runtime stage in
Containerfile and convert the build to a multi-stage setup. Put the dnf install
for python3.11, python3.11-pip, make, and openssh-clients into a builder stage,
then copy only the needed runtime artifacts into the final stage. Keep the final
image free of build-time tooling by updating the relevant build stage and final
stage instructions around the existing RUN dnf install step.
In `@tests/bmaas/conftest.py`:
- Around line 57-62: The catalog-item teardown in the fixture is swallowing
delete failures by catching Exception and only printing a warning, which hides
real cleanup problems. Update the teardown path around
private_grpc.delete_baremetal_instance_catalog_item in the fixture so it still
logs the failure but re-raises the exception afterward, allowing the test to
fail when cleanup of the BareMetalInstanceCatalogItem does not succeed.
In `@tests/bmaas/test_baremetal_instance_lifecycle.py`:
- Around line 89-97: The teardown in the baremetal instance lifecycle test is
incorrectly gated on CR name lookup, so cleanup can be skipped when the CR is
not yet queryable. Update the exception handler around the BMI cleanup path to
always call cli.delete_baremetal_instance using the known bmi_id, and keep only
wait_for_bmi_deletion dependent on whether
k8s_hub_client.get_baremetal_instance_name resolved bmi_cr. Leave
wait_for_bmi_grpc_removal tied to the deletion attempt so the cleanup in
test_baremetal_instance_lifecycle remains robust even when bmi_cr is empty.
In `@tests/core/osac_cli.py`:
- Around line 17-24: The OsacCLI setup leaves behind the temporary config
directory if the initial login fails because __init__ creates _config_dir before
calling _run("login", ...), and close() is never reached on exceptions. Update
the OsacCLI initialization flow to clean up _config_dir when the login attempt
raises, using the existing _config_dir and close() cleanup path or an equivalent
try/finally around the login call so failed fixture setup does not leak
osac-config-* directories.
🪄 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: 3be24af6-f958-4d18-b0d4-a91652ded705
📒 Files selected for processing (10)
.gitignoreContainerfileMakefiletests/bmaas/__init__.pytests/bmaas/conftest.pytests/bmaas/test_baremetal_instance_lifecycle.pytests/core/grpc_client.pytests/core/helpers.pytests/core/k8s_client.pytests/core/osac_cli.py
🛑 Comments failed to post (4)
Containerfile (1)
6-6: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Move package installation out of the final image stage.
Line 6 installs tooling directly into the runtime image, which keeps package-manager/build-time surface in the shipped layer. Split dependency installation into a builder stage and copy only the required runtime artifacts into the final stage. As per path instructions,
**/{Dockerfile,Containerfile}*: “Multi-stage builds; no build tools in final image”.🤖 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 6, Move the package installation out of the final runtime stage in Containerfile and convert the build to a multi-stage setup. Put the dnf install for python3.11, python3.11-pip, make, and openssh-clients into a builder stage, then copy only the needed runtime artifacts into the final stage. Keep the final image free of build-time tooling by updating the relevant build stage and final stage instructions around the existing RUN dnf install step.Source: Path instructions
tests/bmaas/conftest.py (1)
57-62: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't hide catalog-item teardown failures.
This fixture creates a real
BareMetalInstanceCatalogItem, but Lines 57-62 turn any delete failure into a warning. That makes leaked catalog items invisible to CI and will pollute the shared test environment over time. Let teardown fail after logging instead of swallowing the exception.Suggested fix
try: print(f"\nDeleting BareMetalInstanceCatalogItem {item_id}...") private_grpc.delete_baremetal_instance_catalog_item(item_id=item_id) print(f"CatalogItem {item_id} deleted") except Exception as e: - print(f"WARNING: Failed to delete catalog item {item_id}: {e}") + pytest.fail(f"Failed to delete catalog item {item_id}: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try: print(f"\nDeleting BareMetalInstanceCatalogItem {item_id}...") private_grpc.delete_baremetal_instance_catalog_item(item_id=item_id) print(f"CatalogItem {item_id} deleted") except Exception as e: pytest.fail(f"Failed to delete catalog item {item_id}: {e}")🧰 Tools
🪛 Ruff (0.15.20)
[warning] 61-61: Do not catch blind exception:
Exception(BLE001)
🤖 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/bmaas/conftest.py` around lines 57 - 62, The catalog-item teardown in the fixture is swallowing delete failures by catching Exception and only printing a warning, which hides real cleanup problems. Update the teardown path around private_grpc.delete_baremetal_instance_catalog_item in the fixture so it still logs the failure but re-raises the exception afterward, allowing the test to fail when cleanup of the BareMetalInstanceCatalogItem does not succeed.tests/bmaas/test_baremetal_instance_lifecycle.py (1)
89-97: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't gate teardown delete on CR discovery.
If an earlier assertion fails before the BMI CR becomes queryable,
bmi_cris empty here and the handler skipscli.delete_baremetal_instance(...)entirely. That leaks the real BMI/host even thoughbmi_idis already known. Delete bybmi_idunconditionally, and only guardwait_for_bmi_deletion(...)on whether the CR name was resolved.Suggested fix
except BaseException: bmi_cr: str = k8s_hub_client.get_baremetal_instance_name(uuid=bmi_id, checked=False) - if bmi_cr: - try: - cli.delete_baremetal_instance(uuid=bmi_id) + try: + cli.delete_baremetal_instance(uuid=bmi_id) + if bmi_cr: wait_for_bmi_deletion(k8s=k8s_hub_client, name=bmi_cr) - wait_for_bmi_grpc_removal(grpc=grpc, uuid=bmi_id) - except Exception: - pass + wait_for_bmi_grpc_removal(grpc=grpc, uuid=bmi_id) + except Exception: + pass raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.except BaseException: bmi_cr: str = k8s_hub_client.get_baremetal_instance_name(uuid=bmi_id, checked=False) try: cli.delete_baremetal_instance(uuid=bmi_id) if bmi_cr: wait_for_bmi_deletion(k8s=k8s_hub_client, name=bmi_cr) wait_for_bmi_grpc_removal(grpc=grpc, uuid=bmi_id) except Exception: pass🧰 Tools
🪛 Ruff (0.15.20)
[error] 96-97:
try-except-passdetected, consider logging the exception(S110)
[warning] 96-96: Do not catch blind exception:
Exception(BLE001)
🤖 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/bmaas/test_baremetal_instance_lifecycle.py` around lines 89 - 97, The teardown in the baremetal instance lifecycle test is incorrectly gated on CR name lookup, so cleanup can be skipped when the CR is not yet queryable. Update the exception handler around the BMI cleanup path to always call cli.delete_baremetal_instance using the known bmi_id, and keep only wait_for_bmi_deletion dependent on whether k8s_hub_client.get_baremetal_instance_name resolved bmi_cr. Leave wait_for_bmi_grpc_removal tied to the deletion attempt so the cleanup in test_baremetal_instance_lifecycle remains robust even when bmi_cr is empty.tests/core/osac_cli.py (1)
17-24: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clean up the temp config directory when login fails.
Line 20 allocates a new directory before Line 21 performs the first
osac login. If that login raises, the object never reachesclose(), so failed fixture setup leaks anosac-config-*directory on disk.Suggested fix
self._config_dir: str = tempfile.mkdtemp(prefix="osac-config-") - self._run("login", "--address", address, "--insecure", "--token-script", token_script) + try: + self._run("login", "--address", address, "--insecure", "--token-script", token_script) + except Exception: + shutil.rmtree(self._config_dir, ignore_errors=True) + raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# Each OsacCLI instance gets its own config directory so that parallel # xdist workers (or multiple CLI fixtures) don't overwrite each other's # login credentials via the shared ~/.config/osac/config.json. self._config_dir: str = tempfile.mkdtemp(prefix="osac-config-") try: self._run("login", "--address", address, "--insecure", "--token-script", token_script) except Exception: shutil.rmtree(self._config_dir, ignore_errors=True) raise def close(self) -> None: shutil.rmtree(self._config_dir, ignore_errors=True)🤖 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/core/osac_cli.py` around lines 17 - 24, The OsacCLI setup leaves behind the temporary config directory if the initial login fails because __init__ creates _config_dir before calling _run("login", ...), and close() is never reached on exceptions. Update the OsacCLI initialization flow to clean up _config_dir when the login attempt raises, using the existing _config_dir and close() cleanup path or an equivalent try/finally around the login call so failed fixture setup does not leak osac-config-* directories.
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Nick Carboni <ncarboni@redhat.com>
625353c to
4a9e818
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 7: Remove make from the package list in the Containerfile RUN
installation step so the final container image contains no build tools. Preserve
installation of python3.11, python3.11-pip, jq, and openssh-clients, and update
any related command that depends on make to use the underlying test or execution
command directly.
- Around line 47-48: Replace the broad COPY . . instruction with explicit COPY
directives for only the files and directories required by the final image and
tests, such as tests/, infra/, and Makefile. Remove the subsequent RUN rm -f
osac-cli-bin workaround and verify all required build inputs remain available.
🪄 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: 776ef9a5-2707-462d-aa43-02c879c1709e
📒 Files selected for processing (3)
.gitignoreContainerfileMakefile
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 7: Remove make from the package list in the Containerfile RUN
installation step so the final container image contains no build tools. Preserve
installation of python3.11, python3.11-pip, jq, and openssh-clients, and update
any related command that depends on make to use the underlying test or execution
command directly.
- Around line 47-48: Replace the broad COPY . . instruction with explicit COPY
directives for only the files and directories required by the final image and
tests, such as tests/, infra/, and Makefile. Remove the subsequent RUN rm -f
osac-cli-bin workaround and verify all required build inputs remain available.
🪄 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: 776ef9a5-2707-462d-aa43-02c879c1709e
📒 Files selected for processing (3)
.gitignoreContainerfileMakefile
🛑 Comments failed to post (2)
Containerfile (2)
7-7: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Remove build tools from the final container image.
The modified package list retains
make. As per path instructions, build tools must not be present in the final image to minimize the attack surface. Consider using a multi-stage build, or run tests directly viapytestinstead ofmake <target>ifmakeis only being used as an entrypoint.🤖 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 7, Remove make from the package list in the Containerfile RUN installation step so the final container image contains no build tools. Preserve installation of python3.11, python3.11-pip, jq, and openssh-clients, and update any related command that depends on make to use the underlying test or execution command directly.Source: Path instructions
47-48: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Copy specific files instead of the entire context.
The addition of
RUN rm -f osac-cli-binacts as a workaround for copying the entire build context. As per path instructions, copy only the specific files required for the final image instead of usingCOPY . ..🛡️ Proposed fix to limit copied files
Adjust the copied directories below to match only what the tests strictly require (e.g.,
tests/,infra/, andMakefile):-COPY . . -RUN rm -f osac-cli-bin +COPY Makefile ./ +COPY infra/ ./infra/ +COPY tests/ ./tests/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.COPY Makefile ./ COPY infra/ ./infra/ COPY tests/ ./tests/🤖 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` around lines 47 - 48, Replace the broad COPY . . instruction with explicit COPY directives for only the files and directories required by the final image and tests, such as tests/, infra/, and Makefile. Remove the subsequent RUN rm -f osac-cli-bin workaround and verify all required build inputs remain available.Source: Path instructions
|
@coderabbitai the comments you tried to post are not part of this PRs diff and are issues that existed both before and after my change |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 105 --repo osac-project/osac-test-infra --json comments,reviews --jq '.'Length of output: 60588
What happened is that a path-based instruction matching A couple of ways to avoid this going forward:
I'll leave those two Containerfile comments as-is for this PR since they're pre-existing and not something you introduced — no action needed on your end unless you want to revisit the Containerfile structure separately. |
|
|
||
| @pytest.fixture(scope="session") | ||
| def bmi_template() -> str: | ||
| return env("OSAC_BMI_TEMPLATE", "osac.templates.bm_host_metal3_provisioning") |
There was a problem hiding this comment.
| return env("OSAC_BMI_TEMPLATE", "osac.templates.bm_host_metal3_provisioning") | |
| return env("OSAC_BMI_TEMPLATE", "osac.templates.bm_host_provisioning") |
this one is the default now: https://github.com/osac-project/osac-aap/tree/main/collections/ansible_collections/osac/templates/roles/bm_host_provisioning/tasks
adriengentil
left a comment
adriengentil
left a comment
There was a problem hiding this comment.
looks good overall, just a nit
Add tests/bmaas/ with: - conftest: session-scoped catalog item fixture (template via OSAC_BMI_TEMPLATE env var), BMH namespace config - test_baremetal_instance_lifecycle: full provision/deprovision cycle verifying API states and BMH infrastructure (image, consumerRef) - test_baremetal_instance_power_management: run_strategy HALTED/ALWAYS reflected in BMH spec.online Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Nick Carboni <ncarboni@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Nick Carboni <ncarboni@redhat.com>
- Add openssh-clients for ssh-keygen (bmaas test fixture generates SSH keys) - Bump osac CLI to v0.0.68 (includes ssh_key → ssh_public_key rename) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Nick Carboni <ncarboni@redhat.com>
4a9e818 to
ed89555
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/bmaas/test_baremetal_instance_lifecycle.py (1)
96-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog teardown failures instead of silently passing.
Silently catching exceptions during the fallback cleanup can mask resource leaks and make it harder to detect stranded hosts. Consider logging the error, similar to the teardown block in
conftest.py, so developers have context if the environment remains dirty.♻️ Proposed refactor
- except Exception: - pass + except Exception as e: + print(f"WARNING: BMI cleanup failed during teardown: {e}")🤖 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/bmaas/test_baremetal_instance_lifecycle.py` around lines 96 - 97, Update the fallback cleanup exception handler in the bare-metal instance lifecycle test to log the caught teardown error instead of silently passing. Reuse the existing teardown logging approach from conftest.py and preserve the cleanup flow after logging the failure.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@tests/bmaas/test_baremetal_instance_lifecycle.py`:
- Around line 96-97: Update the fallback cleanup exception handler in the
bare-metal instance lifecycle test to log the caught teardown error instead of
silently passing. Reuse the existing teardown logging approach from conftest.py
and preserve the cleanup flow after logging the failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 57f69466-a896-4331-a076-3f43df17efa4
📒 Files selected for processing (6)
.gitignoreContainerfileMakefiletests/bmaas/__init__.pytests/bmaas/conftest.pytests/bmaas/test_baremetal_instance_lifecycle.py
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, carbonin 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
Jira: https://redhat.atlassian.net/browse/OSAC-1561
Story type: [QE]
Summary
Add E2E tests that validate the full BareMetalInstance lifecycle against a real Metal3/BareMetalOperator environment. Tests exercise the tenant-facing osac CLI and gRPC API, then verify infrastructure-level effects on BareMetalHost CRs. Validated against the ostest cluster (2 passed in ~32 minutes).
Changes
Core test utilities (
tests/core/):grpc_client.py: BareMetalInstance CRUD + CatalogItem admin operations (withfield_definitionssupport)osac_cli.py:create_baremetal_instance/delete_baremetal_instanceCLI wrappersk8s_client.py: BareMetalInstance CR queries + BareMetalHost infrastructure queries (explicit namespace)helpers.py: Polling/wait functions for BMI lifecycle and BMH provisioning stateBMaaS test suite (
tests/bmaas/):conftest.py: Session-scoped fixtures — catalog item (template viaOSAC_BMI_TEMPLATE), SSH key generation, BMH namespace configtest_baremetal_instance_lifecycle.py: Full provision/deprovision cycle + power management testREADME.md: Local run guide with prerequisites, execution, cleanup, and troubleshootingCI integration:
Makefile:test-bmaastarget.gitignore:.artifacts/directoryTesting
test_baremetal_instance_lifecycle: Create BMI → RUNNING → verify BMH provisioned/image/consumerRef → delete → verify BMH available/cleanedtest_baremetal_instance_power_management: Create BMI → RUNNING → HALTED (verify BMH offline) → ALWAYS (verify BMH online) → cleanupAcceptance Criteria
AC-1: E2E tests create a BareMetalInstance and validate it transitions to RUNNINGAC-2: E2E tests verify a BareMetalHost is assigned (consumerRef set) and provisioned (image set, state is provisioned)AC-3: E2E tests delete the BareMetalInstance and verify deprovisioning completes (BMH image cleared, consumerRef removed, returns to available)AC-4: E2E tests verify power state management (run_strategy HALTED/ALWAYS reflected in BMH spec.online)AC-5: E2E tests run against a cluster with BareMetalOperator installed (make test-bmaas)AC-6: Error case — no matching host (deferred, requires investigation into catalog-to-hostType mapping)Summary by CodeRabbit
test-bmaasmake target that runs the suite with optional filtering and produces JUnit XML output.