Skip to content

OSAC-1330: update E2E tests for type-safe resource references - #272

Closed
htayrie-rh wants to merge 4 commits into
osac-project:mainfrom
htayrie-rh:feat/OSAC-1330-type-safe-resource-references
Closed

htayrie-rh wants to merge 4 commits into
osac-project:mainfrom
htayrie-rh:feat/OSAC-1330-type-safe-resource-references

Conversation

@htayrie-rh

@htayrie-rh htayrie-rh commented Jul 29, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Wrap all reference fields in typed reference objects ({"id": ...} or {"name": ...}) to match the new wire format from fulfillment-service PR #986
  • Update GRPCClient methods: create_compute_instance, update_restart, create_virtual_network, create_subnet, create_security_group, create_external_ip, create_external_ip_attachment, and all three catalog item create methods
  • Fix test assertions that read back reference fields from gRPC responses (template, catalogItem, pool, external_ip)
  • Fix network_class fixture in catalog conftest to return metadata.name instead of id

Test plan

  • E2E vmaas suite passes with fulfillment-service PR #986
  • E2E catalog suite passes with fulfillment-service PR #986

Depends on osac-project/fulfillment-service#986

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of catalog items and resource references using structured identifiers and names.
    • Preserved validation that unpublished catalog items cannot be used to create resources.
    • Improved cleanup of external IP pools and attachments during test operations.
  • Tests

    • Updated lifecycle coverage for catalog items, clusters, compute instances, networking, and external IP resources.
    • Added assertions aligned with the latest resource representations.

Wrap reference fields in typed reference objects ({id: ...} or
{name: ...}) to match the new wire format from fulfillment-service.

Changes:
- grpc_client.py: wrap all reference fields sent in gRPC requests
- catalog conftest: network_class fixture returns name instead of id
- catalog lifecycle tests: update assertions and inline calls
- external_ip conftest: update pool/attachment cleanup assertions

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
@openshift-ci-robot

openshift-ci-robot commented Jul 29, 2026 •

Copy link
Copy Markdown

@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.

Details

In response to this:

Summary

  • Wrap all reference fields in typed reference objects ({"id": ...} or {"name": ...}) to match the new wire format from fulfillment-service PR #986
  • Update GRPCClient methods: create_compute_instance, update_restart, create_virtual_network, create_subnet, create_security_group, create_external_ip, create_external_ip_attachment, and all three catalog item create methods
  • Fix test assertions that read back reference fields from gRPC responses (template, catalogItem, pool, external_ip)
  • Fix network_class fixture in catalog conftest to return metadata.name instead of id

Test plan

  • E2E vmaas suite passes with fulfillment-service PR #986
  • E2E catalog suite passes with fulfillment-service PR #986

Depends on osac-project/fulfillment-service#986

Assisted-by: Claude Code 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 commented Jul 29, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: htayrie-rh
Once this PR has been reviewed and has the lgtm label, please assign adriengentil for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@coderabbitai

coderabbitai Bot commented Jul 29, 2026 •

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4cbe5938-2335-4e0b-bfc8-1af276518ea4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Walkthrough

The test gRPC client now sends nested resource references and template objects. Catalog lifecycle tests and fixtures validate the corresponding response shapes, while ExternalIP cleanup reads nested reference IDs.

Changes

Nested reference shapes

Layer / File(s) Summary
Update gRPC request payload shapes
tests/core/grpc_client.py
Create and update requests now wrap resource references in id objects and templates or names in name objects.
Align catalog lifecycle fixtures and tests
tests/catalog/*
Catalog tests and fixtures now validate nested template, catalog item, and network class representations.
Update ExternalIP cleanup lookups
tests/vmaas/external_ip/conftest.py
Pool cleanup identifies ExternalIPs and attachments through nested reference IDs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: lgtm, approved


Important

Pre-merge checks failed

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

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
No-Injection-Vectors ❓ Inconclusive placeholder pending analysis
✅ Passed checks (9 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: updating E2E tests to use typed resource references.
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 found; changes only wrap IDs/names and use a token parameter in auth headers, with no secret literals or embedded creds.
No-Weak-Crypto ✅ Passed Touched files only update typed reference payloads/assertions; no MD5/SHA1/DES/RC4/ECB/custom crypto or secret comparisons found.
Container-Privileges ✅ Passed Touched files are only Python tests/client code; scan found no privileged, hostPID/Network/IPC, SYS_ADMIN, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs ✅ Passed No new logging or secret/PII exposure was introduced; the patch only changes request/response shapes and assertions.
Ai-Attribution ✅ Passed HEAD commit has an Assisted-by trailer for Claude Code, and no Co-Authored-By AI trailer appears in the branch.
✨ 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.

@htayrie-rh
htayrie-rh marked this pull request as draft July 29, 2026 14:59
The E2E tests were using "cudn_net" (implementation_strategy format
with underscores) as the network_class value. With type-safe references,
this value is sent as {"name": "cudn_net"} and looked up against
metadata.name, which is "cudn-net" (DNS-label format with hyphens).

Change the default from "cudn_net" to "cudn-net" to match the
NetworkClass's metadata.name.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
…name format

Template fixtures sent Ansible collection IDs (e.g. osac.templates.ocp_virt_vm)
as reference names, but the reference validator now looks up by metadata.name
(DNS-label format, e.g. ocp-virt-vm). Update all fixture defaults and env var
documentation to use metadata.name format.

Signed-off-by: Hector Tayrie <htayrie@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
The type-safe references PR changed template lookups to use
metadata.name (DNS-label format). Update the BMaaS E2E workflow
default from the old dotted ID to the new format.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown

PR needs rebase.

Details

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 kubernetes-sigs/prow repository.

@htayrie-rh

Copy link
Copy Markdown
Member Author

Superseded by #296, which was merged on Aug 5.

This branch was previously deployed

1 inactive deployment
e2e-test — 027ac700 Deployed Jul 30, 2026 by htayrie-rh via e2e-caas-full-install / e2e #106
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.

2 participants