Skip to content

OSAC-839: E2E integration tests for PublicIPAttachment lifecycle#67

Merged
openshift-merge-bot[bot] merged 2 commits into
osac-project:mainfrom
DakCrowder:publicip-lifecycle-attachments-updates
Jun 11, 2026
Merged

OSAC-839: E2E integration tests for PublicIPAttachment lifecycle#67
openshift-merge-bot[bot] merged 2 commits into
osac-project:mainfrom
DakCrowder:publicip-lifecycle-attachments-updates

Conversation

@DakCrowder

@DakCrowder DakCrowder commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

OSAC-839

Implements tests for:

  • Attach / Re-Attach logic between a ComputeInstance and a PublicIP
  • A subset of api level rejections for invalid operations of an IP Attachment

Assisted-by: Claude noreply@anthropic.com

Summary by CodeRabbit

  • Tests
    • Added comprehensive test coverage for Public IP attachment operations: create, retrieve, list, delete, lifecycle (attach → detach → reattach), and validation/error cases.
  • Test fixtures & helpers
    • Added a fixture to provision compute instances and teardown handling.
    • Added gRPC assertion helper and Kubernetes polling helpers to wait for attachment CRs, readiness, and deletion.

Assisted-by: Claude <noreply@anthropic.com>
@openshift-ci-robot

openshift-ci-robot commented Jun 10, 2026

Copy link
Copy Markdown

@DakCrowder: This pull request references OSAC-839 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 sub-task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

OSAC-839

Implements tests for:

  • Attach / Re-Attach logic between a ComputeInstance and a PublicIP
  • A subset of api level rejections for invalid operations of an IP Attachment

Assisted-by: Claude noreply@anthropic.com

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.

@openshift-ci openshift-ci Bot requested review from eranco74 and larsks June 10, 2026 17:26
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds PublicIPAttachment test support: GRPCClient CRUD wrappers and K8s query methods, assertion and polling helpers, a fixture to provision compute instances, and class-based tests for attach/detach/reattach and validation failure cases.

Changes

PublicIPAttachment Testing Infrastructure

Layer / File(s) Summary
gRPC and K8s Client Extensions for PublicIPAttachments
tests/core/grpc_client.py, tests/core/k8s_client.py
GRPCClient gains PublicIPAttachment CRUD methods (create, get, list IDs, delete); K8sClient gains methods to query attachment name and phase via kubectl jsonpath.
Assertion and Polling Helpers for PublicIPAttachments
tests/core/helpers.py
New assert_grpc_rejected validates gRPC error codes in subprocess exceptions; polling helpers wait for attachment CR creation, Ready phase (assert not Failed), and deletion.
Compute Instance Provisioning and Cleanup Fixtures
tests/vmaas/public_ip/conftest.py
Class-scoped make_compute_instances fixture provisions N compute instances via OsacCLI, waits for CR and running state, records (uuid, name) pairs, and deletes remaining instances on teardown.
PublicIPAttachment Lifecycle and Validation Test Cases
tests/vmaas/public_ip/test_public_ip_pool_lifecycle.py
Replaces prior lifecycle function with class-based tests: test_attach_detach_reattach verifies attach→detach→reattach and address persistence; test_validation_rejections asserts gRPC rejects invalid/duplicate attachment requests.
Existing Capacity Test Refactoring
tests/vmaas/public_ip/test_public_ip_pool_capacity.py
Pool capacity and deletion-blocking tests refactored to use assert_grpc_rejected for gRPC failure assertions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

lgtm, approved

Suggested reviewers

  • larsks
  • eranco74
  • adriengentil
  • omer-vishlitzky

Poem

Tests now bind Public IPs with careful art,
Clients and kubectl play their part,
Fixtures spin instances up and down,
Polls wait for Ready without a frown,
Reattach proves the address keeps its heart.


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 2 warnings)

Check name Status Explanation Resolution
No-Sensitive-Data-In-Logs ❌ Error Code in tests/core/grpc_client.py line 127 raises RuntimeError with raw stderr/stdout, risking exposure of tokens, API keys, or error details containing sensitive data. Extract only the gRPC error code from output: e.g., error_code = re.search(r'Code:\s*(\w+)', output); raise RuntimeError(f"Failed: {error_code.group(1) if error_code else 'unknown'}") from e
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Ai-Attribution ⚠️ Warning PR describes AI assistance ("Assisted-by: Claude") but commit b71ef11 lacks required attribution trailer. Red Hat trailers must be present in commits when AI tools are used. Add "Assisted-by: Claude noreply@anthropic.com" trailer to the commit message, or amend/rebase with proper attribution following existing repository patterns.
✅ Passed checks (8 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the primary change: adding E2E integration tests for PublicIPAttachment lifecycle, which is reflected throughout the changeset across multiple test files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed No hardcoded secrets, API keys, tokens, passwords, or credentials found in any modified files. Token handling uses dynamic runtime generation via fixture, not hardcoded values.
No-Weak-Crypto ✅ Passed No weak cryptographic algorithms or secret comparisons detected in the PR code. Contains only test infrastructure and integration tests.
No-Injection-Vectors ✅ Passed No injection vectors found. All subprocess calls use safe argument lists, no shell=True, no eval/exec/pickle/yaml.load/os.system patterns detected in new code.
Container-Privileges ✅ Passed No container/K8s manifests in PR—only Python test code. Check for container privileges does not apply to Python test files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/vmaas/public_ip/test_public_ip_pool_lifecycle.py`:
- Line 115: The test currently calls make_compute_instances(2) but only uses the
first instance; change the call to provision a single instance by calling
make_compute_instances(1) and adjust the unpacking to match a single-item return
(e.g. replace "(ci1_uuid, _ci1_name), *_ = make_compute_instances(2)" with a
single-item unpack like "(ci1_uuid, _ci1_name), = make_compute_instances(1)");
update any related tuple unpacking in the same test to reflect the
single-instance return.
🪄 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: b3171f39-c86c-4f7f-9bdd-7baefd34eafa

📥 Commits

Reviewing files that changed from the base of the PR and between 6fd40a6 and b71ef11.

📒 Files selected for processing (2)
  • tests/vmaas/public_ip/conftest.py
  • tests/vmaas/public_ip/test_public_ip_pool_lifecycle.py
💤 Files with no reviewable changes (1)
  • tests/vmaas/public_ip/conftest.py

Comment thread tests/vmaas/public_ip/test_public_ip_pool_lifecycle.py Outdated
@DakCrowder DakCrowder force-pushed the publicip-lifecycle-attachments-updates branch from b71ef11 to 8e5211f Compare June 10, 2026 19:58

@akshaynadkarni akshaynadkarni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM.

@DakCrowder Make sure you create a ticket for the pending tests under OSAC-859

@openshift-ci

openshift-ci Bot commented Jun 11, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akshaynadkarni, DakCrowder

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot Bot merged commit 5c2b986 into osac-project:main Jun 11, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants