OSAC-2164: ClusterVersion CLI and ResolveFieldPath reuse - #181
Conversation
|
@sk-ilya: This pull request references OSAC-2164 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 story to target the "5.0.0" version, but no target version was set. This pull request references OSAC-2158 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 story 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. |
ada1b1c to
9c82791
Compare
9c82791 to
d21e9b0
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds generic protobuf field-path reflection helpers and migrates metadata extraction to them. It adds ClusterVersion lookup and lifecycle details to cluster descriptions and table rendering. ChangesCluster version and reflection updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterDescribe
participant ClusterVersionsClient
participant ClusterRenderer
ClusterDescribe->>ClusterVersionsClient: Resolve referenced ClusterVersion
ClusterVersionsClient-->>ClusterDescribe: Return version or lookup error
ClusterDescribe->>ClusterRenderer: Render cluster with optional version details
Possibly related PRs
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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fulfillment-service/internal/cmd/cli/describe/cluster/describe_cluster.go (1)
114-165: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate output errors from
renderCluster.Return an error from
renderClusterand propagate allfmtwrite errors and bothtabwriter.Flush()errors torun. Otherwise, the command can return success after a failed output write.🤖 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/cmd/cli/describe/cluster/describe_cluster.go` around lines 114 - 165, Change renderCluster to return an error, check and propagate every fmt write result and both tabwriter.Flush calls, including versionWriter.Flush. Update run to receive and return the renderCluster error so failed output writes cause the command to fail.Sources: Path instructions, Learnings
🤖 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/cmd/cli/describe/cluster/describe_cluster.go`:
- Around line 91-100: Update the ClusterVersions request inside the lookup
callback passed to lookup.Find to explicitly include every supported version
state, including OBSOLETE, and add a lookup-level test covering an obsolete
referenced version. In renderCluster, return errors from both
tabwriter.Writer.Flush calls and propagate those errors to the caller.
---
Outside diff comments:
In `@fulfillment-service/internal/cmd/cli/describe/cluster/describe_cluster.go`:
- Around line 114-165: Change renderCluster to return an error, check and
propagate every fmt write result and both tabwriter.Flush calls, including
versionWriter.Flush. Update run to receive and return the renderCluster error so
failed output writes cause the command to fail.
🪄 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: Enterprise
Run ID: aff66810-c913-4913-a030-7d6ac9e014ce
📒 Files selected for processing (13)
fulfillment-service/internal/auth/grpc_authz_interceptor.gofulfillment-service/internal/cmd/cli/describe/cluster/describe_cluster.gofulfillment-service/internal/cmd/cli/describe/cluster/describe_cluster_test.gofulfillment-service/internal/references/lookups.gofulfillment-service/internal/references/reference_validator.gofulfillment-service/internal/reflection/reflection_field_path.gofulfillment-service/internal/reflection/reflection_field_path_test.gofulfillment-service/internal/reflection/reflection_helper.gofulfillment-service/internal/reflection/reflection_helper_test.gofulfillment-service/internal/rendering/table_renderer.gofulfillment-service/internal/rendering/table_renderer_cluster_version_test.gofulfillment-service/internal/rendering/tables/osac.private.v1.Cluster.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.Cluster.yaml
d21e9b0 to
f1695d7
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.
Actionable comments posted: 2
🤖 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/rendering/table_renderer_cluster_version_test.go`:
- Around line 77-88: Update the Lookup mock in the table renderer test to
explicitly return versionHelper only for the ClusterVersion descriptor, while
retaining clusterHelper for the cluster descriptor. Replace the default fallback
with a test failure for any unexpected object type so invalid lookups cannot
pass.
- Around line 137-145: Update the test case around renderClusters to use a
cluster ID without a hyphen, then assert the VERSION column specifically
contains “-” rather than matching any hyphen in the rendered row. Keep the
no-version_name setup and makeVersionHelper(nil) behavior unchanged.
🪄 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: Enterprise
Run ID: 5fa7f139-3960-41d1-9ba9-ab3af2c88be5
📒 Files selected for processing (13)
fulfillment-service/internal/auth/grpc_authz_interceptor.gofulfillment-service/internal/cmd/cli/describe/cluster/describe_cluster.gofulfillment-service/internal/cmd/cli/describe/cluster/describe_cluster_test.gofulfillment-service/internal/references/lookups.gofulfillment-service/internal/references/reference_validator.gofulfillment-service/internal/reflection/reflection_field_path.gofulfillment-service/internal/reflection/reflection_field_path_test.gofulfillment-service/internal/reflection/reflection_helper.gofulfillment-service/internal/reflection/reflection_helper_test.gofulfillment-service/internal/rendering/table_renderer.gofulfillment-service/internal/rendering/table_renderer_cluster_version_test.gofulfillment-service/internal/rendering/tables/osac.private.v1.Cluster.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.Cluster.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- fulfillment-service/internal/reflection/reflection_helper.go
- fulfillment-service/internal/rendering/tables/osac.private.v1.Cluster.yaml
- fulfillment-service/internal/auth/grpc_authz_interceptor.go
- fulfillment-service/internal/rendering/tables/osac.public.v1.Cluster.yaml
- fulfillment-service/internal/references/lookups.go
- fulfillment-service/internal/cmd/cli/describe/cluster/describe_cluster.go
- fulfillment-service/internal/rendering/table_renderer.go
- fulfillment-service/internal/cmd/cli/describe/cluster/describe_cluster_test.go
- fulfillment-service/internal/references/reference_validator.go
- fulfillment-service/internal/reflection/reflection_field_path.go
- fulfillment-service/internal/reflection/reflection_field_path_test.go
- fulfillment-service/internal/reflection/reflection_helper_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fulfillment-service/internal/rendering/table_renderer_cluster_version_test.go (1)
130-131: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe negative assertion on line 131 can never fail.
The renderer writes through a
tabwriter.Writerthat is configured with' 'as the pad character.Flushreplaces the tab separators with spaces, so the rendered output contains no\t.ContainSubstring("4-17-0\t")therefore always passes and proves nothing about the VERSION cell.Assert the cell content directly, in the same way as the
'-'test on lines 141-145.💚 Proposed assertion
output := renderClusters(ctx, cluster, makeVersionHelper([]proto.Message{version})) - Expect(output).To(ContainSubstring("4.17.0")) - Expect(output).ToNot(ContainSubstring("4-17-0\t")) + lines := strings.Split(strings.TrimSpace(output), "\n") + Expect(lines).To(HaveLen(2)) + versionCol := strings.Index(lines[0], "VERSION") + Expect(versionCol).To(BeNumerically(">=", 0)) + Expect(strings.Fields(lines[1][versionCol:])[0]).To(Equal("4.17.0"))🤖 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/rendering/table_renderer_cluster_version_test.go` around lines 130 - 131, Replace the ineffective tab-based negative assertion in the table renderer test with a direct assertion on the VERSION cell, matching the existing '-' test pattern around the relevant cell content. Verify that the rendered VERSION value is “4.17.0” and not the hyphenated form, without relying on tab separators after tabwriter flushes.
🧹 Nitpick comments (2)
fulfillment-service/internal/rendering/table_renderer.go (1)
523-524: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe
registriesmap is unsynchronized shared state in a CEL binding.
lookupFunctioncapturesregistriesand mutates it inside the overload binding. TodayrenderRowruns sequentially, so this is safe.cel.Programvalues are otherwise safe for concurrent evaluation, so the shared map is a trap for a future parallel render. Add a short comment that the renderer is single-goroutine, or guard the map with a mutex.Also applies to: 553-572
🤖 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/rendering/table_renderer.go` around lines 523 - 524, Add synchronization for the shared registries map captured by TableRenderer.lookupFunction, protecting mutations in the overload binding with a mutex so concurrent CEL evaluations remain safe. Use the existing lookupFunction closure and its registry-update path; do not change the lookup behavior.fulfillment-service/internal/rendering/table_renderer_cluster_version_test.go (1)
166-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test rebuilds the mock helper wiring instead of reusing
renderClusters.The setup on lines 166-182 duplicates the builder block on lines 103-113, and its
Lookupstub silently returnsnilfor unexpected types while the shared stub callsFail. Extract the renderer construction into a small helper so both tests share one strictLookupbehavior.🤖 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/rendering/table_renderer_cluster_version_test.go` around lines 166 - 175, Extract the renderer construction currently duplicated around renderClusters and the test setup into a shared test helper. Reuse that helper in both tests, preserving the strict Lookup behavior that calls Fail for unexpected object types instead of returning nil.
🤖 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/rendering/table_renderer.go`:
- Around line 516-586: Update lookupFunction to degrade gracefully when
r.helper.Lookup returns nil or type-registry creation/descriptor registration
fails: log a warning with the target type and error context, then return
registry.NativeToValue(helper.Instance()) instead of types.NewErr. Preserve
normal lookup and key-miss behavior, and ensure the fallback uses the empty
instance for the resolved target type.
---
Outside diff comments:
In
`@fulfillment-service/internal/rendering/table_renderer_cluster_version_test.go`:
- Around line 130-131: Replace the ineffective tab-based negative assertion in
the table renderer test with a direct assertion on the VERSION cell, matching
the existing '-' test pattern around the relevant cell content. Verify that the
rendered VERSION value is “4.17.0” and not the hyphenated form, without relying
on tab separators after tabwriter flushes.
---
Nitpick comments:
In
`@fulfillment-service/internal/rendering/table_renderer_cluster_version_test.go`:
- Around line 166-175: Extract the renderer construction currently duplicated
around renderClusters and the test setup into a shared test helper. Reuse that
helper in both tests, preserving the strict Lookup behavior that calls Fail for
unexpected object types instead of returning nil.
In `@fulfillment-service/internal/rendering/table_renderer.go`:
- Around line 523-524: Add synchronization for the shared registries map
captured by TableRenderer.lookupFunction, protecting mutations in the overload
binding with a mutex so concurrent CEL evaluations remain safe. Use the existing
lookupFunction closure and its registry-update path; do not change the lookup
behavior.
🪄 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: Enterprise
Run ID: a47e9c17-efba-4c44-917c-c61a02acd82f
📒 Files selected for processing (24)
fulfillment-service/internal/rendering/table_renderer.gofulfillment-service/internal/rendering/table_renderer_cluster_version_test.gofulfillment-service/internal/rendering/table_renderer_compute_instance_test.gofulfillment-service/internal/rendering/tables/osac.private.v1.BareMetalInstance.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.Cluster.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.ComputeInstance.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.ExternalIP.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.ExternalIPAttachment.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.ExternalIPPool.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.NATGateway.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.RoleBinding.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.SecurityGroup.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.Subnet.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.User.yamlfulfillment-service/internal/rendering/tables/osac.private.v1.VirtualNetwork.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.BareMetalInstance.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.Cluster.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.ComputeInstance.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.NATGateway.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.RoleBinding.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.SecurityGroup.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.Subnet.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.User.yamlfulfillment-service/internal/rendering/tables/osac.public.v1.VirtualNetwork.yaml
💤 Files with no reviewable changes (19)
- fulfillment-service/internal/rendering/tables/osac.public.v1.NATGateway.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.BareMetalInstance.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.ComputeInstance.yaml
- fulfillment-service/internal/rendering/tables/osac.public.v1.SecurityGroup.yaml
- fulfillment-service/internal/rendering/tables/osac.public.v1.BareMetalInstance.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.User.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.NATGateway.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.Subnet.yaml
- fulfillment-service/internal/rendering/tables/osac.public.v1.RoleBinding.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.RoleBinding.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.ExternalIPPool.yaml
- fulfillment-service/internal/rendering/tables/osac.public.v1.ComputeInstance.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.ExternalIP.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.ExternalIPAttachment.yaml
- fulfillment-service/internal/rendering/tables/osac.public.v1.User.yaml
- fulfillment-service/internal/rendering/tables/osac.public.v1.VirtualNetwork.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.SecurityGroup.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.VirtualNetwork.yaml
- fulfillment-service/internal/rendering/tables/osac.public.v1.Subnet.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- fulfillment-service/internal/rendering/tables/osac.public.v1.Cluster.yaml
- fulfillment-service/internal/rendering/tables/osac.private.v1.Cluster.yaml
| // lookupFunction returns a CEL environment option that registers the lookup() function. The function | ||
| // takes a protobuf reference message (e.g. ClusterVersionReference) and returns the corresponding | ||
| // object (e.g. ClusterVersion) so that the CEL expression can navigate its fields. | ||
| // | ||
| // Each lookup target type gets its own CEL type registry, created lazily on first use and cached | ||
| // for the lifetime of the Render call. This avoids eagerly registering every resource type in the | ||
| // CEL environment. | ||
| func (r *TableRenderer) lookupFunction(ctx context.Context) cel.EnvOption { | ||
| registries := map[protoreflect.FullName]*types.Registry{} | ||
|
|
||
| return cel.Function("lookup", | ||
| cel.Overload( | ||
| "lookup_ref", | ||
| []*cel.Type{cel.DynType}, | ||
| cel.DynType, | ||
| cel.UnaryBinding(func(val ref.Val) ref.Val { | ||
| msg, ok := val.Value().(proto.Message) | ||
| if !ok { | ||
| return types.NewErr("lookup: expected a protobuf reference message, got %T", val.Value()) | ||
| } | ||
| targetType, ok := referenceTargetType(msg.ProtoReflect().Descriptor()) | ||
| if !ok { | ||
| r.logger.ErrorContext( | ||
| ctx, | ||
| "Failed to derive lookup target type from reference message", | ||
| slog.String("message", string(msg.ProtoReflect().Descriptor().FullName())), | ||
| ) | ||
| return types.NewErr( | ||
| "lookup: cannot derive target type from %q", | ||
| msg.ProtoReflect().Descriptor().FullName(), | ||
| ) | ||
| } | ||
| helper := r.helper.Lookup(string(targetType)) | ||
| if helper == nil { | ||
| return types.NewErr("lookup: unknown target type %q", targetType) | ||
| } | ||
|
|
||
| registry, ok := registries[targetType] | ||
| if !ok { | ||
| var err error | ||
| registry, err = types.NewRegistry() | ||
| if err != nil { | ||
| return types.NewErr( | ||
| "lookup: failed to create type registry for %q: %v", | ||
| targetType, err, | ||
| ) | ||
| } | ||
| for _, fd := range pb.CollectFileDescriptorSet(helper.Instance()) { | ||
| if err = registry.RegisterDescriptor(fd); err != nil { | ||
| return types.NewErr( | ||
| "lookup: failed to register type %q: %v", | ||
| targetType, err, | ||
| ) | ||
| } | ||
| } | ||
| registries[targetType] = registry | ||
| } | ||
|
|
||
| key := referenceKey(msg) | ||
| if key == "" { | ||
| return registry.NativeToValue(helper.Instance()) | ||
| } | ||
| object := r.resolveObject(ctx, targetType, key) | ||
| if object == nil { | ||
| return registry.NativeToValue(helper.Instance()) | ||
| } | ||
| return registry.NativeToValue(object) | ||
| }), | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A missing helper or registry failure now aborts the whole table.
lookup() returns types.NewErr for an unknown target type and for registry failures. renderRow propagates the evaluation error, so Render fails and the user sees no rows at all. The rest of the renderer degrades gracefully on purpose: Render falls back to the default table on load failure, and lookupName falls back to the key. Keep that behavior here and return the empty instance instead, with a logged warning.
🛡️ Proposed graceful fallback
helper := r.helper.Lookup(string(targetType))
if helper == nil {
- return types.NewErr("lookup: unknown target type %q", targetType)
+ r.logger.WarnContext(
+ ctx,
+ "Failed to find object helper for lookup target type",
+ slog.String("type", string(targetType)),
+ )
+ return types.NewErr("lookup: unknown target type %q", targetType)
}Alternatively, return a dynamic empty message of targetType so the caller expression still evaluates.
🤖 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/rendering/table_renderer.go` around lines 516 -
586, Update lookupFunction to degrade gracefully when r.helper.Lookup returns
nil or type-registry creation/descriptor registration fails: log a warning with
the target type and error context, then return
registry.NativeToValue(helper.Instance()) instead of types.NewErr. Preserve
normal lookup and key-miss behavior, and ensure the fallback uses the empty
instance for the resolved target type.
9ef7777 to
632d197
Compare
|
@jhernand I had to rebase, could you please approve once again? |
Add VERSION column to cluster list tables (public and private) that resolves version_name to the semver string via a new lookup_field option in the table renderer. Extend describe cluster to fetch and display ClusterVersion details in a nested section. Move the field path resolver to the reflection package as a reusable utility. Assisted-by: Claude <noreply@anthropic.com>
… walkers Replace four manual protobuf field-walking implementations with calls to the generic ResolveFieldPath[T] / ResolveFieldPathOr[T] helpers in the reflection package. Break the test-time import cycle by replacing the config.TenantFromContext dependency in reflection tests with test-local context helpers. Assisted-by: Claude <noreply@anthropic.com>
Replace the explicit lookup boolean with auto-detection via the proto registry, and introduce a lazy per-target CEL type registry for the lookup() function using pb.CollectFileDescriptorSet for complete transitive import coverage. - Remove Lookup bool and LookupField from columnLayout; renderCell now auto-detects message types via FindMessageByName (same as enums use FindEnumByName) - Remove lookup: true from all 21 YAML table definitions - Change VERSION column to use lookup(this.spec.version).spec.version CEL expression instead of the old lookup_field mechanism - Add lookup() CEL function with lazy type registries created per target type on first use, avoiding eager registration of all resource types - Extract resolveObject as shared cache+fetch core for both lookupName and lookupFunction - Add referenceTargetType and referenceKey helpers for the lookup() convention - Add test for cross-file type navigation via lookup() Assisted-by: Claude <noreply@anthropic.com>
632d197 to
fa64064
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, omer-vishlitzky, sk-ilya 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 |
Assisted-by: Claude noreply@anthropic.com
Summary by CodeRabbit