Repository navigation
OSAC-1330: Update E2E tests for typed resource references - #296
Conversation
|
@htayrie-rh: This pull request references OSAC-1330 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 feature 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (12)
WalkthroughThe pull request updates template and network identifiers, changes resource references to nested API objects, and adjusts lifecycle tests and external-IP cleanup for the new representations. ChangesResource reference alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 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/test_jwt_auth_smoke.py`:
- Line 77: Add the network_class: str fixture parameter to all three smoke tests
and replace each hardcoded "cudn-net" argument in create_virtual_network calls
with the injected network_class value, preserving the fixture-driven
OSAC_NETWORK_CLASS behavior.
🪄 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: Pro Plus
Run ID: 204d85ca-0214-4b2f-bc1a-04a76ed69173
📒 Files selected for processing (13)
.github/workflows/e2e-bmaas-full-install.ymlAGENTS.mdCLAUDE.mdtests/bmaas/conftest.pytests/caas/conftest.pytests/catalog/conftest.pytests/catalog/test_catalog_item_lifecycle.pytests/catalog/test_compute_instance_catalog_item_lifecycle.pytests/core/grpc_client.pytests/storage/conftest.pytests/vmaas/conftest.pytests/vmaas/external_ip/conftest.pytests/vmaas/test_jwt_auth_smoke.py
| vn_name: str = f"jwt-smoke-{uuid4().hex[:8]}" | ||
| vn_id: str = jwt_grpc_tenant1.create_virtual_network( | ||
| name=vn_name, network_class="cudn_net", ipv4_cidr="10.200.0.0/16" | ||
| name=vn_name, network_class="cudn-net", ipv4_cidr="10.200.0.0/16" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the network_class fixture in all three smoke tests.
The network_class fixture in tests/vmaas/conftest.py reads OSAC_NETWORK_CLASS, but these calls pass "cudn-net" directly. A valid deployment with another network class will fail because the override is ignored. Add network_class: str to each test and pass it to create_virtual_network.
Proposed fix
-def test_jwt_virtual_network_lifecycle(jwt_grpc_tenant1: GRPCClient) -> None:
+def test_jwt_virtual_network_lifecycle(
+ jwt_grpc_tenant1: GRPCClient, network_class: str
+) -> None:
...
- name=vn_name, network_class="cudn-net", ipv4_cidr="10.200.0.0/16"
+ name=vn_name, network_class=network_class, ipv4_cidr="10.200.0.0/16"
-def test_jwt_security_group_lifecycle(jwt_grpc_tenant1: GRPCClient) -> None:
+def test_jwt_security_group_lifecycle(
+ jwt_grpc_tenant1: GRPCClient, network_class: str
+) -> None:
...
- name=vn_name, network_class="cudn-net", ipv4_cidr="10.202.0.0/16"
+ name=vn_name, network_class=network_class, ipv4_cidr="10.202.0.0/16"
-def test_jwt_tenant_isolation(jwt_grpc_tenant1: GRPCClient, jwt_grpc_tenant2: GRPCClient) -> None:
+def test_jwt_tenant_isolation(
+ jwt_grpc_tenant1: GRPCClient, jwt_grpc_tenant2: GRPCClient, network_class: str
+) -> None:
...
- name=vn_name, network_class="cudn-net", ipv4_cidr="10.201.0.0/16"
+ name=vn_name, network_class=network_class, ipv4_cidr="10.201.0.0/16"Based on learnings, configure environment-dependent behavior through documented environment variables so local development and CI behave consistently.
Also applies to: 90-90, 115-115
🤖 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_jwt_auth_smoke.py` at line 77, Add the network_class: str
fixture parameter to all three smoke tests and replace each hardcoded "cudn-net"
argument in create_virtual_network calls with the injected network_class value,
preserving the fixture-driven OSAC_NETWORK_CLASS behavior.
Source: Learnings
6888f9b to
311b02c
Compare
Update all gRPC client calls and test fixtures to use typed reference
objects (e.g., {"id": value} or {"name": value}) instead of raw strings,
matching the fulfillment-service API changes for type-safe resource
references.
- Update grpc_client.py to pass typed reference dicts for subnet,
virtual_network, pool, external_ip, compute_instance, catalog_item,
and template fields
- Update default template/network_class values from underscore to
hyphenated names (e.g., osac.templates.ocp_virt_vm → ocp-virt-vm)
- Update test assertions to access nested reference fields
- Update AGENTS.md and CLAUDE.md documentation
Depends on osac-project/osac#85
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
311b02c to
783daf1
Compare
|
Looks good. Depends on: osac-project/osac#85 |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danielerez, htayrie-rh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…use metadata.name (#321) PR #296 updated the test code to use typed references but missed the workflow input defaults. The vm-template default was still osac.templates.ocp_virt_vm (template id format) instead of ocp-virt-vm (metadata.name). Same for cluster-template in CaaS. The typed reference validation looks up templates by name, so passing the old id format causes "ComputeInstanceTemplate not found" errors. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb> Co-authored-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
This branch had an error being deployed
4 participants
Summary
{"id": value}/{"name": value}) instead of raw stringsosac.templates.ocp_virt_vm→ocp-virt-vm)obj["template"]→obj["template"]["name"])Test plan
Related
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
Compatibility
Documentation
Tests