OSAC-3954: populate Name on resolved ClusterTemplateReference and HostTypeReference by omer-vishlitzky · Pull Request #277 · osac-project/osac

omer-vishlitzky · 2026-08-11T23:04:53Z

Summary

Set Name (from metadata.name) alongside Id on resolved ClusterTemplateReference and HostTypeReference in validateAndTransformCluster
Previously only Id was set, leaving Name empty — metering reads Name for the cluster_template billing dimension, which was always ""
Fix covers all three resolution sites: create path (line 760), signal/update path (line 198), and update validation path (line 986)
Added test assertions verifying Name is preserved after reference resolution

Root Cause

private_clusters_server.go resolves user-provided template/host-type references to their canonical form (by ID). The resolved ClusterTemplateReference was constructed with only SetId(), dropping the Name field. Downstream, osac-metering's ClusterBillingDimensions() reads spec.GetTemplate().GetName() for the cluster_template billing dimension — always empty.

Test plan

Existing cluster tests pass (ginkgo run --focus="Cluster" internal/servers — 115 passed)
Added assertions in "Creates object with template specified by name" and "Creates object with template and host type specified by name" tests verifying GetName() returns expected value after resolution
CaaS metering E2E tests (osac-test-infra PR #349) should pass once this lands in the image

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

Bug Fixes
- Private cluster creation now consistently retains the names of selected templates and host types, whether chosen by name, ID, or through a catalog item.
- Catalog-item-based creation now correctly records the resolved template details before applying defaults.

…tTypeReference validateAndTransformCluster only set Id on resolved references, leaving Name empty. Metering reads Name for the cluster_template billing dimension, which was always empty as a result. Set Name from metadata.name on all three reference resolution sites (create, update, and signal paths). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>

openshift-ci-robot · 2026-08-11T23:04:57Z

@omer-vishlitzky: This pull request references OSAC-3954 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 bug to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

Set Name (from metadata.name) alongside Id on resolved ClusterTemplateReference and HostTypeReference in validateAndTransformCluster

Previously only Id was set, leaving Name empty — metering reads Name for the cluster_template billing dimension, which was always ""

Fix covers all three resolution sites: create path (line 760), signal/update path (line 198), and update validation path (line 986)

Added test assertions verifying Name is preserved after reference resolution

Root Cause

private_clusters_server.go resolves user-provided template/host-type references to their canonical form (by ID). The resolved ClusterTemplateReference was constructed with only SetId(), dropping the Name field. Downstream, osac-metering's ClusterBillingDimensions() reads spec.GetTemplate().GetName() for the cluster_template billing dimension — always empty.

Test plan

Existing cluster tests pass (ginkgo run --focus="Cluster" internal/servers — 115 passed)

Added assertions in "Creates object with template specified by name" and "Creates object with template and host type specified by name" tests verifying GetName() returns expected value after resolution

CaaS metering E2E tests (osac-test-infra PR #349) should pass once this lands in the image

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.

coderabbitai · 2026-08-11T23:05:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: 8896ec51-f652-4f42-bd2d-b0e01ae39f59

📥 Commits

Reviewing files that changed from the base of the PR and between 9cf778c and 199714a.

📒 Files selected for processing (2)

fulfillment-service/internal/servers/private_clusters_server.go
fulfillment-service/internal/servers/private_clusters_server_test.go

Walkthrough

Private cluster creation now persists resolved template and host-type references with both identifiers and metadata names. Catalog-item creation explicitly resolves template metadata before applying defaults. Tests cover name- and identifier-based resolution.

Changes

Private cluster reference persistence

Layer / File(s)	Summary
Persist resolved reference metadata `fulfillment-service/internal/servers/private_clusters_server.go`, `fulfillment-service/internal/servers/private_clusters_server_test.go`	Standard creation flows retain template and host-type metadata names with their identifiers. Tests cover name- and identifier-based resolution.
Resolve catalog-item references `fulfillment-service/internal/servers/private_clusters_server.go`, `fulfillment-service/internal/servers/private_clusters_server_test.go`	Catalog-item creation resolves template and host-type names before storing the cluster references. Tests verify the resolved names and identifiers.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

osac-project/osac#43: Updates private cluster tests for persisted metadata names.
osac-project/osac#84: Modifies private cluster and catalog-item creation flows.
osac-project/osac#183: Updates typed cluster reference resolution and metadata persistence.

Suggested reviewers: tzumainn, danmanor

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: populating names on resolved cluster template and host type references.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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	The PR adds only metadata-name assignments, reference handling, comments, and test assertions; no API keys, tokens, passwords, private keys, credential URLs, or secret-shaped blobs were introduced.
No-Weak-Crypto	✅ Passed	The cumulative PR diff adds reference-name setters, tests, and comments only; scans found no added MD5, SHA1, DES, RC4, Blowfish, ECB, custom crypto, or secret comparison logic.
No-Injection-Vectors	✅ Passed	The PR adds only reference-name assignments and test assertions; the added-line scan found no SQL concatenation, eval/exec, shell, pickle, YAML, or HTML injection vector.
Container-Privileges	✅ Passed	The PR changes only two Go files and tests; the diff adds no container/Kubernetes manifest or prohibited privilege setting.
No-Sensitive-Data-In-Logs	✅ Passed	The PR diff adds only reference names, assignments, comments, and test assertions; it adds no logging or sensitive-data output. Existing catalog lookup logging is unchanged.
Ai-Attribution	✅ Passed	AI use is disclosed, and both PR commits contain `Assisted-by: Claude Code <noreply@anthropic.com>` trailers. No AI `Co-Authored-By` marker exists in the PR range.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Create stacked PR
Commit on current branch

🧪 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.}

fullsend-ai-review · 2026-08-11T23:06:16Z

🤖 Finished Review · ✅ Success · Started 11:06 PM UTC · Completed 11:20 PM UTC

Commit: 1c8b46f · View workflow run →

fullsend-ai-review · 2026-08-11T23:20:10Z

Looks good to me

Previous run

Review

Findings

High

[logic-error] fulfillment-service/internal/servers/private_clusters_server.go:931 — The validateAndTransformCatalogItem function sets the cluster's template reference from the catalog item (catalogItem.GetTemplate()) at line 931 but never rebuilds it with the resolved template metadata name. The template is looked up at line 940, making template.GetMetadata().GetName() available, but this name is never set on the cluster's template reference. In contrast, validateAndTransformCluster (lines 762–764) correctly builds a resolvedTemplateRef with both SetId() and SetName(). Clusters created via the catalog item path will still have an empty cluster_template billing dimension in metering.
Remediation: After the template lookup and deletion check, rebuild the template reference similar to validateAndTransformCluster:
```
resolvedTemplateRef := &privatev1.ClusterTemplateReference{}
resolvedTemplateRef.SetId(template.GetId())
resolvedTemplateRef.SetName(template.GetMetadata().GetName())
cluster.GetSpec().SetTemplate(resolvedTemplateRef)
```
Also add a test assertion for the catalog item creation path verifying GetSpec().GetTemplate().GetName() is populated.

Low

[test-inadequate] fulfillment-service/internal/servers/private_clusters_server_test.go:351 — The tests "Creates object with host type specified by name in node set" and "Creates object with host type specified by identifier in node set" do not assert that GetHostType().GetName() is populated after resolution. The PR adds name assertions only in the combined template+host-type test case.
[comment-typo] fulfillment-service/internal/servers/private_clusters_server_test.go:415 — The comment contains "the the template" (doubled word). The PR modified this comment but did not fix the pre-existing typo.
Remediation: Change "Verify that the the template" to "Verify that the template".

Labels: PR fixes a bug in the fulfillment-service cluster provisioning logic

Next steps:

/fs-fix — agent addresses review findings automatically
/fs-fix <your instruction> — agent fixes with your specific guidance
Push commits directly — review re-runs automatically on push
/fs-fix-stop — disable automatic fix runs for this PR

Address fullsend review findings: - Set Name on resolved ClusterTemplateReference in validateAndTransformCatalogItem (catalog item creation path was still missing it) - Add Name assertions to host-type-by-name, host-type-by-id, and catalog-item tests - Fix "the the" typo in test comment Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>

fullsend-ai-review · 2026-08-12T00:37:23Z

🤖 Finished Review · ✅ Success · Started 12:37 AM UTC · Completed 12:50 AM UTC

Commit: 199714a · View workflow run →

Superseded by updated review

omer-vishlitzky · 2026-08-12T04:54:25Z

/retest

github-actions · 2026-08-12T04:54:43Z

Re-triggered failed runs:

label-gate (#31551611425)
E2E BMaaS Full Install (#31550759187)
label-gate (#31550758873)

omer-vishlitzky · 2026-08-12T07:05:38Z

/retest

github-actions · 2026-08-12T07:05:57Z

Re-triggered failed runs:

label-gate (#31551611425)
E2E BMaaS Full Install (#31550759187)
label-gate (#31550758873)

masayag

Reviewed with a focus on whether hostType.GetMetadata()/template.GetMetadata() can ever be nil across the resolution paths touched here (Create, validateAndTransformCluster, validateAndTransformCatalogItem).

Confirmed safe in all flows:

All four GetMetadata() call sites are already behind an explicit hostType != nil / template != nil check.
Even without that, the generated protobuf accessors (GetMetadata(), GetName()) are nil-receiver safe (if x != nil { ... }; return nil/""), so the chain can't panic regardless.
generic_dao_get.go / generic_dao_list.go always construct and attach a non-nil Metadata for any row successfully hydrated from the DB, so a non-nil hostType/template returned from lookupHostType/lookupTemplate always has real metadata, not just a nil-safe empty string.

One non-blocking nit:

validateTemplateImmutability()'s comment (around L598-600, unchanged by this PR) says:

// Compare by refKey (Id) rather than proto.Equal because validateAndTransformCluster normalizes the stored reference to Id-only, while the reference validator interceptor may backfill Name on incoming requests.

That's now stale — after this change, validateAndTransformCluster/validateAndTransformCatalogItem set both Id and Name on the resolved reference, not Id-only. Behavior is unaffected (refKey() prefers Id when present), but worth a follow-up comment update so it doesn't mislead future readers.

LGTM otherwise.

openshift-ci · 2026-08-12T07:26:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: masayag, omer-vishlitzky

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:

~~fulfillment-service/OWNERS~~ [masayag,omer-vishlitzky]

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

openshift-ci-robot added the jira/valid-reference label Aug 11, 2026

openshift-ci Bot requested review from danmanor and tzumainn August 11, 2026 23:04

openshift-ci Bot added the approved label Aug 11, 2026

omer-vishlitzky enabled auto-merge August 11, 2026 23:05

omer-vishlitzky temporarily deployed to e2e-test August 11, 2026 23:05 — with GitHub Actions Inactive

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added the bug Something isn't working label Aug 11, 2026

omer-vishlitzky temporarily deployed to e2e-test August 12, 2026 00:36 — with GitHub Actions Inactive

omer-vishlitzky had a problem deploying to e2e-test August 12, 2026 00:36 — with GitHub Actions Failure

coderabbitai Bot approved these changes Aug 12, 2026

View reviewed changes

fullsend-ai-review Bot approved these changes Aug 12, 2026

View reviewed changes

fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Aug 12, 2026

omer-vishlitzky had a problem deploying to e2e-test August 12, 2026 04:54 — with GitHub Actions Failure

omer-vishlitzky had a problem deploying to e2e-test August 12, 2026 07:06 — with GitHub Actions Failure

masayag approved these changes Aug 12, 2026

View reviewed changes

openshift-ci Bot assigned masayag Aug 12, 2026

openshift-ci Bot added the lgtm label Aug 12, 2026

omer-vishlitzky disabled auto-merge August 12, 2026 08:04

omer-vishlitzky merged commit 8675130 into osac-project:main Aug 12, 2026
99 of 110 checks passed

omer-vishlitzky mentioned this pull request Aug 12, 2026

OSAC-3954: fix stale comment — reference resolution now sets Id and Name #280

Merged

Uh oh!

Conversation

omer-vishlitzky commented Aug 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Test plan

Summary by CodeRabbit

Uh oh!

openshift-ci-robot commented Aug 11, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Test plan

Uh oh!

coderabbitai Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

fullsend-ai-review Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

High

Low

Uh oh!

This comment was marked as outdated.

Uh oh!

fullsend-ai-review Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omer-vishlitzky commented Aug 12, 2026

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

omer-vishlitzky commented Aug 12, 2026

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

masayag left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Aug 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

omer-vishlitzky commented Aug 11, 2026 •

edited by coderabbitai Bot

Loading

openshift-ci-robot commented Aug 11, 2026 •

edited by openshift-ci Bot

Loading

coderabbitai Bot commented Aug 11, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 11, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 11, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 12, 2026 •

edited

Loading