OSAC-3675: Convert version_name to typed ClusterVersionReference - #183
Merged
openshift-merge-bot[bot] merged 3 commits intoAug 6, 2026
Conversation
|
@htayrie-rh: This pull request references OSAC-3675 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 epic 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. |
WalkthroughThe PR replaces ChangesCluster version reference migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PrivateClustersServer
participant ClusterVersionDAO
participant Database
Client->>PrivateClustersServer: Submit spec.version
PrivateClustersServer->>ClusterVersionDAO: Validate or resolve version
ClusterVersionDAO-->>PrivateClustersServer: Return ClusterVersionReference
PrivateClustersServer->>Database: Persist cluster with version reference
Database-->>PrivateClustersServer: Return persisted cluster
PrivateClustersServer-->>Client: Return cluster
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
![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
`@fulfillment-service/internal/controllers/cluster/cluster_reconciler_function.go`:
- Around line 346-348: Update
fulfillment-service/internal/controllers/cluster/cluster_reconciler_function.go:346-348
and the resolveVersionImage, lookupAndValidateClusterVersion, and catalog-item
extractor flows to resolve ClusterVersionReference by id when present, while
validating and propagating project and shared scope fields; preserve name lookup
as the fallback. Update
fulfillment-service/internal/servers/private_cluster_templates_server.go:163-170
to pass every reference field through, and
fulfillment-service/internal/servers/private_cluster_catalog_items_server.go:167-193
to include project/shared in catalog-item JSON defaults; add coverage for
ID-only and scoped references.
In
`@fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql`:
- Around line 66-83: The two field_definitions rebuilds in
fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql
at lines 66-83 and 93-110 must preserve array order. Update both
jsonb_array_elements aggregations to use WITH ORDINALITY and add ORDER BY ord
inside jsonb_agg, including the archived catalog items update.
In `@fulfillment-service/internal/servers/private_clusters_server.go`:
- Around line 357-365: Update
fulfillment-service/internal/servers/private_clusters_server.go:357-365 to
resolve ClusterVersionReference using both id and name, reject conflicting
values, and persist the canonical resolved reference; update 604-613 to compare
and validate the complete reference rather than preserving the existing version
when name is empty. Add Create and Update coverage for ID-only references.
In `@fulfillment-service/proto/public/osac/public/v1/cluster_type.proto`:
- Around line 153-162: Preserve the deprecated string field tags and assign the
new typed ClusterVersionReference fields unused tags, with server-side
translation during the compatibility window. Update
fulfillment-service/proto/public/osac/public/v1/cluster_type.proto (lines
153-162),
fulfillment-service/proto/public/osac/public/v1/cluster_template_type.proto
(lines 127-131),
fulfillment-service/proto/private/osac/private/v1/cluster_type.proto (line 43),
and
fulfillment-service/proto/private/osac/private/v1/cluster_template_type.proto
(lines 66-67): retain tag 6 for deprecated cluster version strings and tag 5 for
deprecated template version strings respectively; do not reuse those tags for
the typed protobuf fields.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 104ad672-2436-4765-96fd-67d0cbf76c74
⛔ Files ignored due to path filters (12)
fulfillment-service/internal/api/osac/private/v1/cluster_template_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_template_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_version_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_version_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_template_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_template_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_version_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_version_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (25)
fulfillment-service/internal/cmd/cli/create/cluster/create_cluster_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/reference_lookups.gofulfillment-service/internal/controllers/cluster/cluster_reconciler_function.gofulfillment-service/internal/controllers/cluster/cluster_reconciler_function_test.gofulfillment-service/internal/database/migrations.sha256fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sqlfulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference_test.gofulfillment-service/internal/servers/catalog_item_validation_test.gofulfillment-service/internal/servers/cluster_version_resolution.gofulfillment-service/internal/servers/clusters_server_test.gofulfillment-service/internal/servers/private_cluster_catalog_items_server.gofulfillment-service/internal/servers/private_cluster_catalog_items_server_test.gofulfillment-service/internal/servers/private_cluster_templates_server.gofulfillment-service/internal/servers/private_cluster_templates_server_test.gofulfillment-service/internal/servers/private_clusters_server.gofulfillment-service/internal/servers/private_clusters_server_test.gofulfillment-service/internal/utils/cluster_spec_defaults.gofulfillment-service/internal/utils/cluster_spec_defaults_test.gofulfillment-service/it/it_public_clusters_test.gofulfillment-service/proto/private/osac/private/v1/cluster_template_type.protofulfillment-service/proto/private/osac/private/v1/cluster_type.protofulfillment-service/proto/private/osac/private/v1/cluster_version_type.protofulfillment-service/proto/public/osac/public/v1/cluster_template_type.protofulfillment-service/proto/public/osac/public/v1/cluster_type.protofulfillment-service/proto/public/osac/public/v1/cluster_version_type.proto
Comment on lines
+346
to
+348
| // Resolve version reference to a release image via the ClusterVersion resource. | ||
| if versionRef := clusterSpec.GetVersion(); versionRef != nil && versionRef.GetName() != "" { | ||
| image, err := t.resolveVersionImage(ctx, versionRef.GetName()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'message ClusterVersionReference|ClusterVersionReference' fulfillment-service/proto
rg -n -C 5 'func refKey|func lookupAndValidateClusterVersion|func \(.*\) resolveVersionImage' fulfillment-service/internal
rg -n -C 4 'GetVersion\(\).*GetName|ClusterVersionReference.*Id|ClusterVersionReference.*Project' fulfillment-service/internalRepository: osac-project/osac
Length of output: 36206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cluster_version_resolution.go =="
sed -n '1,140p' fulfillment-service/internal/servers/cluster_version_resolution.go
echo
echo "== catalog_item_validation.go =="
sed -n '1,130p' fulfillment-service/internal/servers/catalog_item_validation.go
echo
echo "== relevant files sections =="
sed -n '320,390p' fulfillment-service/internal/controllers/cluster/cluster_reconciler_function.go
sed -n '145,180p' fulfillment-service/internal/servers/private_cluster_templates_server.go
sed -n '140,205p' fulfillment-service/internal/servers/private_cluster_catalog_items_server.goRepository: osac-project/osac
Length of output: 15175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== catalog_item_validation.go version handling =="
sed -n '130,240p' fulfillment-service/internal/servers/catalog_item_validation.go
echo
echo "== list/ID lookup in cluster version API =="
rg -n -C 4 'ClusterVersionsListRequest|ClusterVersionReference|GetId|metadata\.id|id:' fulfillment-service/internal internal/api fulfillment-service/protoRepository: osac-project/osac
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== catalog_item_validation.go version handling =="
sed -n '130,240p' fulfillment-service/internal/servers/catalog_item_validation.go
echo
echo "== catalog/cluster tests for id/project/shared versions or validation =="
rg -n -C 3 'ClusterVersionReference|version.*id|version.*project|id.*version|versionName.*non-string|StringV|NullV|list filter' fulfillment-service/internal/servers -g '*test.go' -g '*.go' | head -n 320Repository: osac-project/osac
Length of output: 38899
Resolve every field in ClusterVersionReference.
ClusterVersionReference defines id, name, project, and shared, but the current paths use only name. This skips lookup for ID-only references and can apply the same-name version from the wrong scope, or no filter from the wrong project. Update resolveVersionImage, lookupAndValidateClusterVersion, and the catalog-item extractor to use id when present and to include project/shared in validation and catalog-item JSON defaults. Add coverage for ID-only and scoped references.
📍 Affects 3 files
fulfillment-service/internal/controllers/cluster/cluster_reconciler_function.go#L346-L348(this comment)fulfillment-service/internal/servers/private_cluster_templates_server.go#L163-L170fulfillment-service/internal/servers/private_cluster_catalog_items_server.go#L167-L193
🤖 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
`@fulfillment-service/internal/controllers/cluster/cluster_reconciler_function.go`
around lines 346 - 348, Update
fulfillment-service/internal/controllers/cluster/cluster_reconciler_function.go:346-348
and the resolveVersionImage, lookupAndValidateClusterVersion, and catalog-item
extractor flows to resolve ClusterVersionReference by id when present, while
validating and propagating project and shared scope fields; preserve name lookup
as the fallback. Update
fulfillment-service/internal/servers/private_cluster_templates_server.go:163-170
to pass every reference field through, and
fulfillment-service/internal/servers/private_cluster_catalog_items_server.go:167-193
to include project/shared in catalog-item JSON defaults; add coverage for
ID-only and scoped references.
Comment on lines
+66
to
+83
| set data = jsonb_set(data, '{field_definitions}', | ||
| (select coalesce(jsonb_agg( | ||
| case | ||
| when fd->>'path' = 'version_name' then | ||
| case | ||
| when fd->'default' is not null and jsonb_typeof(fd->'default') = 'string' then | ||
| jsonb_set( | ||
| jsonb_set(fd, '{path}', '"version"'), | ||
| '{default}', | ||
| jsonb_build_object('name', fd->>'default') | ||
| ) | ||
| else | ||
| jsonb_set(fd, '{path}', '"version"') | ||
| end | ||
| else fd | ||
| end | ||
| ), '[]'::jsonb) | ||
| from jsonb_array_elements(data->'field_definitions') as fd) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve field_definitions array order.
jsonb_agg has no ORDER BY. Both updates rebuild field_definitions, so PostgreSQL can emit catalog fields in a different sequence. Preserve each element ordinal when aggregating.
fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql#L66-L83: usejsonb_array_elements(...) WITH ORDINALITYand addORDER BY ordinsidejsonb_agg.fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql#L93-L110: apply the same ordered aggregation to archived catalog items.
Proposed fix
- (select coalesce(jsonb_agg(
+ (select coalesce(jsonb_agg(
case
...
- end
+ end order by ord
), '[]'::jsonb)
- from jsonb_array_elements(data->'field_definitions') as fd)
+ from jsonb_array_elements(data->'field_definitions')
+ with ordinality as element(fd, ord))
)📝 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.
Suggested change
| set data = jsonb_set(data, '{field_definitions}', | |
| (select coalesce(jsonb_agg( | |
| case | |
| when fd->>'path' = 'version_name' then | |
| case | |
| when fd->'default' is not null and jsonb_typeof(fd->'default') = 'string' then | |
| jsonb_set( | |
| jsonb_set(fd, '{path}', '"version"'), | |
| '{default}', | |
| jsonb_build_object('name', fd->>'default') | |
| ) | |
| else | |
| jsonb_set(fd, '{path}', '"version"') | |
| end | |
| else fd | |
| end | |
| ), '[]'::jsonb) | |
| from jsonb_array_elements(data->'field_definitions') as fd) | |
| set data = jsonb_set(data, '{field_definitions}', | |
| (select coalesce(jsonb_agg( | |
| case | |
| when fd->>'path' = 'version_name' then | |
| case | |
| when fd->'default' is not null and jsonb_typeof(fd->'default') = 'string' then | |
| jsonb_set( | |
| jsonb_set(fd, '{path}', '"version"'), | |
| '{default}', | |
| jsonb_build_object('name', fd->>'default') | |
| ) | |
| else | |
| jsonb_set(fd, '{path}', '"version"') | |
| end | |
| else fd | |
| end order by ord | |
| ), '[]'::jsonb) | |
| from jsonb_array_elements(data->'field_definitions') | |
| with ordinality as element(fd, ord)) |
📍 Affects 1 file
fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql#L66-L83(this comment)fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql#L93-L110
🤖 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
`@fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql`
around lines 66 - 83, The two field_definitions rebuilds in
fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql
at lines 66-83 and 93-110 must preserve array order. Update both
jsonb_array_elements aggregations to use WITH ORDINALITY and add ORDER BY ord
inside jsonb_agg, including the archived catalog items update.
Comment on lines
+357
to
+365
| versionRef := cluster.GetSpec().GetVersion() | ||
| if versionRef != nil && versionRef.GetName() != "" { | ||
| return lookupAndValidateClusterVersion(ctx, s.logger, s.clusterVersionsDao, versionRef.GetName()) | ||
| } | ||
| versionName, err := resolveDefaultClusterVersionName(ctx, s.logger, s.clusterVersionsDao) | ||
| ref, err := resolveDefaultClusterVersion(ctx, s.logger, s.clusterVersionsDao) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| cluster.GetSpec().SetVersionName(versionName) | ||
| cluster.GetSpec().SetVersion(ref) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the complete ClusterVersionReference identity.
Both paths use only name, although the reference contract contains id and name. An ID-only reference selects the system default during Create and restores the existing value during Update.
fulfillment-service/internal/servers/private_clusters_server.go#L357-L365: resolve an explicit version by ID or name, reject conflicting values, and persist the canonical resolved reference.fulfillment-service/internal/servers/private_clusters_server.go#L604-L613: compare and validate the complete reference instead of treating an empty name as a request to preserve the existing version.
Add Create and Update coverage for ID-only references.
📍 Affects 1 file
fulfillment-service/internal/servers/private_clusters_server.go#L357-L365(this comment)fulfillment-service/internal/servers/private_clusters_server.go#L604-L613
🤖 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 `@fulfillment-service/internal/servers/private_clusters_server.go` around lines
357 - 365, Update
fulfillment-service/internal/servers/private_clusters_server.go:357-365 to
resolve ClusterVersionReference using both id and name, reject conflicting
values, and persist the canonical resolved reference; update 604-613 to compare
and validate the complete reference rather than preserving the existing version
when name is empty. Add Create and Update coverage for ID-only references.
Comment on lines
+153
to
+162
| // Reference to a ClusterVersion resource. | ||
| // | ||
| // The version is resolved with the following precedence: | ||
| // | ||
| // 1. Explicit `spec.version_name` provided by the user. | ||
| // 2. Template `spec_defaults.version_name` (template path) or catalog item field definition default (catalog-item | ||
| // path). | ||
| // 1. Explicit `spec.version` provided by the user. | ||
| // 2. Template `spec_defaults.version` (template path) or catalog item field definition default (catalog-item path). | ||
| // 3. System default (`is_default = true`). | ||
| // | ||
| // The referenced ClusterVersion must exist, be enabled, and not be in `OBSOLETE` state. | ||
| optional string version_name = 6; | ||
| ClusterVersionReference version = 6; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'version_name\s*=\s*[56]|ClusterVersionReference\s+version\s*=\s*[56]' \
fulfillment-service/proto
fd -HI 'buf.yaml' 'buf.work.yaml' 'buf.gen.yaml' '.bufbreaking.yaml' .Repository: osac-project/osac
Length of output: 3127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- buf.yaml ---\n'
cat -n fulfillment-service/buf.yaml
printf '\n--- cluster_type proto snippets ---\n'
cat -n fulfillment-service/proto/public/osac/public/v1/cluster_type.proto | sed -n '130,175p'
cat -n fulfillment-service/proto/private/osac/private/v1/cluster_type.proto | sed -n '25,55p'
printf '\n--- cluster_template_type proto snippets ---\n'
cat -n fulfillment-service/proto/public/osac/public/v1/cluster_template_type.proto | sed -n '110,138p'
cat -n fulfillment-service/proto/private/osac/private/v1/cluster_template_type.proto | sed -n '55,72p'
printf '\n--- cluster templates definitions ---\n'
rg -n -C 2 'message ClusterVersionReference|message .*Cluster.*version_name|optional string version_name|version_name' fulfillment-service/proto
printf '\n--- migration relevant lines ---\n'
cat -n fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql | sed -n '1,150p'
printf '\n--- buf breaking config mentions ---\n'
fd -HI 'buf\.ya?ml|breaking|protovalidate|validate' fulfillment-service | xargs -r rg -n 'breaking|deprecated|PROTO_COMPAT|UNARY|SERVICE|Field|Message' || trueRepository: osac-project/osac
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- buf.yaml ---'
cat -n fulfillment-service/buf.yaml
echo
echo '--- cluster_type proto snippets ---'
sed -n '130,175p' fulfillment-service/proto/public/osac/public/v1/cluster_type.proto | cat -n
sed -n '25,55p' fulfillment-service/proto/private/osac/private/v1/cluster_type.proto | cat -n
echo
echo '--- cluster_template_type proto snippets ---'
sed -n '110,138p' fulfillment-service/proto/public/osac/public/v1/cluster_template_type.proto | cat -n
sed -n '55,72p' fulfillment-service/proto/private/osac/private/v1/cluster_template_type.proto | cat -n
echo
echo '--- cluster templates definitions ---'
rg -n -C 2 'message ClusterVersionReference|message .*Cluster.*version_name|optional string version_name|version_name' fulfillment-service/proto
echo
echo '--- migration relevant lines ---'
sed -n '1,150p' fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql | cat -n
echo
echo '*breaking rules and protobreaking config* ---'
fd -HI 'buf\.ya?ml$|breaking' fulfillment-service | while read -r f; do
echo "--- $f ---"
rg -n 'breaking|deprecated|PROTO_COMPAT|UNARY|SERVICE|Field|Message|PROTO_MESSAGE_DEFAULT' "$f" || true
doneRepository: osac-project/osac
Length of output: 13278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- historical version_name occurrences ---'
git log --all --oneline -- fulfillment-service/proto fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql | sed -n '1,80p'
echo
echo '--- search for version_name across repo ---'
rg -n 'version_name' .
echo
echo '--- inspect git diff for proto schema change if available ---'
git diff --stat HEAD~1..HEAD fulfillment-service/proto fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql || true
echo
echo '--- parse current tags/types for relevant messages ---'
python3 - <<'PY'
from pathlib import Path
import re
files = [
"fulfillment-service/proto/public/osac/public/v1/cluster_type.proto",
"fulfillment-service/proto/private/osac/private/v1/cluster_type.proto",
"fulfillment-service/proto/public/osac/public/v1/cluster_template_type.proto",
"fulfillment-service/proto/private/osac/private/v1/cluster_template_type.proto",
]
for path in files:
text = Path(path).read_text()
print(f"\n{path}:")
for m in re.finditer(r'^(?:optional\s+)?(optional\s+)?(message\s+)?(?:string|int32|google\.protobuf\.Any|Cluster\w+)\s+([^=\s]+)\s*=\s*(\d+)', text, re.M):
fieldspec, _, name, tag = m.groups()
# print full matched line for context clarity
line = text[:m.start()].count('\n') + 1
if name in ("version", "version_name"):
print(f"line {line}: {m.group(0)}")
PYRepository: osac-project/osac
Length of output: 15131
Do not reuse the existing field tags for a different protobuf type.
A deployed client can send version_name as UTF-8 text at tag 5 or 6. The new server parses that text as ClusterVersionReference bytes, so the migration-only JSONB conversion does not cover gRPC or ProtoJSON clients during upgrade. Keep the deprecated string field on its current tag and add the typed field at a new tag with server-side translation during the compatibility window.
fulfillment-service/proto/public/osac/public/v1/cluster_type.proto: retain tag6for the deprecated string field.fulfillment-service/proto/public/osac/public/v1/cluster_template_type.proto: retain tag5for the deprecated string field.fulfillment-service/proto/private/osac/private/v1/cluster_type.proto: retain tag6for the deprecated string field.fulfillment-service/proto/private/osac/private/v1/cluster_template_type.proto: retain tag5for the deprecated string field.
📍 Affects 4 files
fulfillment-service/proto/public/osac/public/v1/cluster_type.proto#L153-L162(this comment)fulfillment-service/proto/public/osac/public/v1/cluster_template_type.proto#L127-L131fulfillment-service/proto/private/osac/private/v1/cluster_type.proto#L43-L43fulfillment-service/proto/private/osac/private/v1/cluster_template_type.proto#L66-L67
🤖 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 `@fulfillment-service/proto/public/osac/public/v1/cluster_type.proto` around
lines 153 - 162, Preserve the deprecated string field tags and assign the new
typed ClusterVersionReference fields unused tags, with server-side translation
during the compatibility window. Update
fulfillment-service/proto/public/osac/public/v1/cluster_type.proto (lines
153-162),
fulfillment-service/proto/public/osac/public/v1/cluster_template_type.proto
(lines 127-131),
fulfillment-service/proto/private/osac/private/v1/cluster_type.proto (line 43),
and
fulfillment-service/proto/private/osac/private/v1/cluster_template_type.proto
(lines 66-67): retain tag 6 for deprecated cluster version strings and tag 5 for
deprecated template version strings respectively; do not reuse those tags for
the typed protobuf fields.
Contributor
|
/approve |
added 2 commits
August 6, 2026 18:43
Replace the last plain-string resource reference in the API: ClusterSpec.version_name (optional string) becomes ClusterSpec.version (ClusterVersionReference message with id, name, project, shared fields). This brings ClusterVersion references in line with all other typed references (HostTypeReference, ClusterTemplateReference, etc.), enabling the reference validation interceptor to auto-populate missing fields and validate existence on incoming requests. Changes: - Add ClusterVersionReference message to public/private protos - Update ClusterSpec and ClusterTemplateSpecDefaults field types - Register ClusterVersionReference in the reference validation interceptor - Update server logic, reconciler, CLI, and spec defaults application - Add SQL migration 93 to backfill JSONB data (string → reference object) - Update all unit and integration tests Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
The CI enforces that every migration file has a corresponding test. Add tests covering cluster, template, and catalog item conversions plus skip-when-absent scenarios. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Haim Tayrie <htayrie@htayrie-thinkpadt14gen5.raanaii.csb>
htayrie-rh
force-pushed
the
feat/OSAC-3675-cluster-version-reference
branch
from
August 6, 2026 15:43
15f1e5a to
18e2301
Compare
|
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fulfillment-service/internal/utils/cluster_spec_defaults_test.go (1)
49-61: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the complete
ClusterVersionReferencein these tests.Both cases set and assert only
Name. PopulateId,Project, andShared, then verify all fields. In the default case, mutatespec.GetVersion()after applying defaults and verify thatdefaults.GetVersion()is unchanged. This protects the new pointer-message cloning contract.Also applies to: 74-86
🤖 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 `@fulfillment-service/internal/utils/cluster_spec_defaults_test.go` around lines 49 - 61, Update the ClusterSpec default tests around ApplyClusterSpecDefaults to populate and assert ClusterVersionReference.Name, Id, Project, and Shared in both cases. In the default-value test, mutate spec.GetVersion() after applying defaults and verify defaults.GetVersion() remains unchanged, covering pointer-message cloning without altering unrelated assertions.
🤖 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.
Nitpick comments:
In `@fulfillment-service/internal/utils/cluster_spec_defaults_test.go`:
- Around line 49-61: Update the ClusterSpec default tests around
ApplyClusterSpecDefaults to populate and assert ClusterVersionReference.Name,
Id, Project, and Shared in both cases. In the default-value test, mutate
spec.GetVersion() after applying defaults and verify defaults.GetVersion()
remains unchanged, covering pointer-message cloning without altering unrelated
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: be1e731e-3f2f-412e-a981-d21c91aa280c
⛔ Files ignored due to path filters (12)
fulfillment-service/internal/api/osac/private/v1/cluster_template_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_template_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_version_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/cluster_version_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_template_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_template_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_version_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/cluster_version_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (25)
fulfillment-service/internal/cmd/cli/create/cluster/create_cluster_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/reference_lookups.gofulfillment-service/internal/controllers/cluster/cluster_reconciler_function.gofulfillment-service/internal/controllers/cluster/cluster_reconciler_function_test.gofulfillment-service/internal/database/migrations.sha256fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sqlfulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference_test.gofulfillment-service/internal/servers/catalog_item_validation_test.gofulfillment-service/internal/servers/cluster_version_resolution.gofulfillment-service/internal/servers/clusters_server_test.gofulfillment-service/internal/servers/private_cluster_catalog_items_server.gofulfillment-service/internal/servers/private_cluster_catalog_items_server_test.gofulfillment-service/internal/servers/private_cluster_templates_server.gofulfillment-service/internal/servers/private_cluster_templates_server_test.gofulfillment-service/internal/servers/private_clusters_server.gofulfillment-service/internal/servers/private_clusters_server_test.gofulfillment-service/internal/utils/cluster_spec_defaults.gofulfillment-service/internal/utils/cluster_spec_defaults_test.gofulfillment-service/it/it_public_clusters_test.gofulfillment-service/proto/private/osac/private/v1/cluster_template_type.protofulfillment-service/proto/private/osac/private/v1/cluster_type.protofulfillment-service/proto/private/osac/private/v1/cluster_version_type.protofulfillment-service/proto/public/osac/public/v1/cluster_template_type.protofulfillment-service/proto/public/osac/public/v1/cluster_type.protofulfillment-service/proto/public/osac/public/v1/cluster_version_type.proto
🚧 Files skipped from review as they are similar to previous changes (23)
- fulfillment-service/internal/utils/cluster_spec_defaults.go
- fulfillment-service/internal/database/migrations.sha256
- fulfillment-service/internal/servers/clusters_server_test.go
- fulfillment-service/internal/controllers/cluster/cluster_reconciler_function.go
- fulfillment-service/internal/servers/private_cluster_templates_server.go
- fulfillment-service/it/it_public_clusters_test.go
- fulfillment-service/internal/servers/private_cluster_templates_server_test.go
- fulfillment-service/proto/public/osac/public/v1/cluster_template_type.proto
- fulfillment-service/internal/controllers/cluster/cluster_reconciler_function_test.go
- fulfillment-service/internal/cmd/cli/create/cluster/create_cluster_cmd.go
- fulfillment-service/internal/servers/catalog_item_validation_test.go
- fulfillment-service/proto/public/osac/public/v1/cluster_type.proto
- fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference.up.sql
- fulfillment-service/proto/public/osac/public/v1/cluster_version_type.proto
- fulfillment-service/internal/servers/private_cluster_catalog_items_server_test.go
- fulfillment-service/proto/private/osac/private/v1/cluster_version_type.proto
- fulfillment-service/internal/servers/private_clusters_server.go
- fulfillment-service/proto/private/osac/private/v1/cluster_template_type.proto
- fulfillment-service/internal/servers/private_cluster_catalog_items_server.go
- fulfillment-service/proto/private/osac/private/v1/cluster_type.proto
- fulfillment-service/internal/servers/cluster_version_resolution.go
- fulfillment-service/internal/database/migrations/93_convert_version_name_to_version_reference_test.go
- fulfillment-service/internal/servers/private_clusters_server_test.go
Contributor
|
/approve |
Contributor
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, danielerez, htayrie-rh 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 was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OSAC-3675: Convert version_name to typed ClusterVersionReference
Jira: https://redhat.atlassian.net/browse/OSAC-3675
Parent: https://redhat.atlassian.net/browse/OSAC-1330 (Type-safe resource references)
Summary
Replaces the last plain-string resource reference in the API:
ClusterSpec.version_name(optional string) becomesClusterSpec.version(ClusterVersionReferencemessage). This brings ClusterVersion references in line with all other typed references (HostTypeReference, ClusterTemplateReference, etc.), enabling the reference validation interceptor to auto-populate missing fields and validate existence on incoming requests.Changes
Proto definitions:
ClusterVersionReferencemessage (id, name, project, shared) to both public and privatecluster_version_type.protoClusterSpecfield 6 fromoptional string version_nametoClusterVersionReference versionClusterTemplateSpecDefaultsfield 5 similarlybuf generateServer logic:
ensureClusterVersionto accept/returnClusterVersionReferencevalidateVersionNameUpdate→validateVersionUpdate,validateSpecDefaultsVersionName→validateSpecDefaultsVersion,validateFieldDefinitionsVersionName→validateFieldDefinitionsVersion{"name": "..."}) instead of stringresolveDefaultClusterVersionNamewrapperReference validation interceptor:
ClusterVersionReferencefor both public and private proto namespaces inreference_lookups.goReconciler:
CLI:
create cluster --versionto construct aClusterVersionReferenceSpec defaults:
proto.Clonewith the reference typeDatabase migration:
"version_name": "4-17-0"→"version": {"name": "4-17-0"})Testing
it_public_clusters_test.goto useClusterVersionReferenceuv run dev.py lintpasses clean (golangci-lint + buf lint)Notes
meta/osac.yaml) still containspec_defaults.version_nameentries, but these are already dead data — the Python filter plugin'sClusterTemplateSpecDefaultsclass does not model that field. A follow-up should addversionsupport to both the Python model and the YAML files.Summary by CodeRabbit
New Features
Validation
Migration