Skip to content

(fix): Bump control-plane for multi-resource catalog - #32

Open
jenniferubah wants to merge 8 commits into
dcm-project:mainfrom
jenniferubah:fix/update-deps
Open

jenniferubah wants to merge 8 commits into
dcm-project:mainfrom
jenniferubah:fix/update-deps

Conversation

@jenniferubah

@jenniferubah jenniferubah commented Sep 16, 2026

Copy link
Copy Markdown

Summary

  • Bump github.com/dcm-project/control-plane to a version with the multi-resource catalog schema
  • Remove top level service type field from catalog item schema
  • Add create tests that assert those fields reach the API

Fixes:
https://redhat.atlassian.net/browse/FLPATH-4853
https://redhat.atlassian.net/browse/FLPATH-4854

Assisted-By: Cursor AI

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Support multi-resource catalog payloads

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Upgrade control-plane schemas to support multi-resource catalog items and instances.
• Verify resource definitions and resource-scoped user values reach the catalog API.
Diagram

sequenceDiagram
  actor User
  participant CLI as Catalog CLI
  participant Schema as Generated Schema
  participant Client as API Client
  participant API as Control Plane
  User->>CLI: Create from YAML
  CLI->>Schema: Parse resource fields
  Schema-->>CLI: Typed request
  CLI->>Client: Submit request
  Client->>API: POST catalog payload
  API-->>Client: Creation response
  Client-->>CLI: Return result
Loading
High-Level Assessment

Upgrading the generated control-plane contract is the appropriate approach because the CLI already parses and submits its generated request types directly. Manually adding parallel payload structures or sending untyped maps would duplicate the API schema and risk future contract drift; focused serialization assertions appropriately guard the dependency-driven fix.

Files changed (4) +43 / -23

Tests (2) +22 / -2
catalog_instance_test.goVerify resource-scoped instance values are serialized +13/-2

Verify resource-scoped instance values are serialized

• Extends instance creation fixtures with a resource-scoped user value. The create test now verifies that catalog item ID, resource, path, and value fields reach the API request body.

internal/commands/catalog_instance_test.go

catalog_item_test.goVerify catalog item resources reach the API +9/-0

Verify catalog item resources reach the API

• Extends the catalog item create test to assert the generated request contains the resource name and service type under spec.resources.

internal/commands/catalog_item_test.go

Other (2) +21 / -21
go.modUpgrade control-plane schema dependency +7/-7

Upgrade control-plane schema dependency

• Bumps the control-plane module to a revision containing multi-resource catalog request schemas. Updates related golang.org/x indirect dependencies selected by the module graph.

go.mod

go.sumRefresh checksums for upgraded modules +14/-14

Refresh checksums for upgraded modules

• Replaces checksums for the previous control-plane revision and records the updated indirect golang.org/x module versions.

go.sum

Assisted-By: Cursor AI

Signed-off-by: Jennifer Ubah <[email protected]>
@qodo-code-review

qodo-code-review Bot commented Sep 16, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Multi-resource items hide service types ⊘ Outdated 🐞 Bug ≡ Correctness
Description
catalogItemTableDef.RowFunc reads only spec.resources[0].service_type and emits a single
service-type cell. When the newly supported schema returns multiple resources, create, list, and get
table output omit every service type after the first while structured output retains them.
Code

go.mod[7]

+	github.com/dcm-project/control-plane v0.0.0-20260805143143-76ca1d7a6639
Evidence
The upgraded dependency activates multi-resource catalog items, while the shared table formatter
explicitly indexes only the first resource. All catalog-item table commands use this formatter, and
the added test fixture contains only one resource, so the omission remains untested.

go.mod[5-8]
internal/commands/catalog_item.go[15-35]
internal/commands/catalog_item.go[61-100]
internal/commands/catalog_item.go[123-170]
internal/commands/catalog_item.go[188-216]
internal/commands/catalog_item_test.go[23-30]
internal/commands/catalog_item_test.go[89-102]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The upgraded catalog schema supports multiple resources, but default catalog-item table output displays only the first resource's service type.

## Fix Focus Areas
- internal/commands/catalog_item.go[15-35]
- internal/commands/catalog_item_test.go[23-30]

## Recommended Fix
Update the catalog-item table row formatter to collect and render the service type from every resource, using an unambiguous plural header or formatted summary. Add create, list, and get coverage using an item with at least two resources.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Catalog examples show the old schema ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
README.md still places catalog-item service_type and fields directly under spec and omits
resource from catalog-instance user values, while the upgraded create tests send spec.resources
entries and resource-qualified values. Users preparing --from-file input from either example
therefore submit the obsolete single-resource shape instead of the multi-resource payload exercised
against the API.
Code

internal/commands/catalog_item_test.go[R91-92]

+				resources, ok := spec["resources"].([]any)
+				Expect(ok).To(BeTrue(), "request body must include spec.resources")
Evidence
Compliance rule 2696907 requires behavior and API specification changes to be reflected in README or
AI documentation. The new assertions in the catalog-item create test establish that items serialize
a spec.resources array whose entries include resource names, service types, and fields, while the
catalog-instance create test establishes that user values serialize a resource field; the
corresponding README examples still show top-level item fields and resource-less user values.

Rule 2696907: Update AI documentation when changing behavior, config, or architecture
internal/commands/catalog_item_test.go[89-97]
internal/commands/catalog_instance_test.go[85-95]
README.md[425-444]
README.md[508-520]
internal/commands/catalog_item_test.go[89-102]
internal/commands/catalog_instance_test.go[85-100]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The dependency bump and updated create tests use multi-resource catalog payloads, but the catalog-item and catalog-instance README examples still document the previous single-resource structure.

## Fix Focus Areas
- README.md[425-444]
- README.md[508-520]
- internal/commands/catalog_item_test.go[89-102]
- internal/commands/catalog_instance_test.go[85-100]

## Recommended Fix
Rewrite the catalog-item example to use `spec.resources` entries containing resource names, service types, and their fields. Add the matching `resource` identifier to every catalog-instance `user_values` entry, preferably demonstrating values for multiple resources so both examples match the payloads exercised by the create tests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials
✅ Compliance rules (platform): 16 rules
Review mode: ⚖️ Balanced: This is a localized dependency and test change, but it alters runtime dependency resolution and API payload coverage, warranting a careful single-pass review.

Grey Divider

Tip of the day
💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread internal/commands/catalog_item_test.go
Comment thread go.mod Outdated
Assisted-By: Cursor AI

Signed-off-by: Jennifer Ubah <[email protected]>
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread internal/commands/catalog_item.go
Assisted-By: Cursor AI

Signed-off-by: Jennifer Ubah <[email protected]>
Comment thread go.mod Outdated
require (
github.com/coreos/go-oidc/v3 v3.20.0
github.com/dcm-project/control-plane v0.0.0-20260814135831-7c173134e932
github.com/dcm-project/control-plane v0.0.0-20260805143143-76ca1d7a6639

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change downgrades github.com/dcm-project/control-plane from the base branch’s 2026-08-14 commit to the older 2026-08-05 commit. The intervening history includes pagination, Helm credential/RBAC, and runtime-output changes, so this is not a dependency bump and may regress behavior already present on main. Please pin the current or a newer control-plane version.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it to 2026-08-17 commit (ef5cf90) and not the latest version because the provider API is removed. That will be fixed in another PR

@jordigilh jordigilh Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will be fixed in another PR

another PR? Could you include it in this PR? At this point we should be mindful to reduce PRs and not expand. As of today I've calculated still open around 48 PRs across all dcm repos. Each PR require 2 engineers to review and approve plus address the changes (if found).

Repository Open PRs PR numbers
cli 1 #32
cnpg-database-service-provider 2 #2, #3
control-plane 8 #61, #62, #63, #64, #65, #66, #68, #70
dcm 9 #109–#117
dcm-project.github.io 2 #8, #22
enhancements 10 #43, #57, #58, #60, #94, #99, #104, #106, #107, #108
environment-agent 7 #34–#40
k8s-network-service-provider 2 #2, #3
k8s-storage-service-provider 2 #8, #9
shared-workflows 1 #37
utilities 4 #37, #44, #45, #49
Total 48

@jenniferubah jenniferubah Sep 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can expand the scope of this PR. Will update it.
From your PR list, I don't think these repos should not be included:

  • dcm (used and managed by Chris)
  • k8s-network-service-provider - embedded, in-flight
  • k8s-storage-service-provider - embedded
  • cnpg-database-service-provider - embedded

Either way, I agree, we have a lot

@jenniferubah jenniferubah Sep 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I checked the changes involved to update it to the latest and replace the provider API with the agent. It is a lot of changes that touches a lot of files which will slow down the review of this bugfix and block QE. So I'd rather do the changes in another PR, I won't create ithe PR right now as it is non blocking, while we reduce our PR backlog. Opened a jira ticket to track this: https://redhat.atlassian.net/browse/FLPATH-4895

Comment thread README.md
Assisted-By: Cursor AI

Signed-off-by: Jennifer Ubah <[email protected]>
Assisted-By: Cursor AI

Signed-off-by: Jennifer Ubah <[email protected]>

@jordigilh jordigilh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second review summary

Substantive findings are posted inline below. The existing approval targets an older commit and does not establish approval of the current head. No direct cross-PR conflict was identified; the control-plane dependency and CLI examples must be validated together.

Verdict: Not GA-ready

Comment thread README.md Outdated
```
ID UID DISPLAY NAME CREATED
my-catalog-item b2c3d4e5-f6a7-8901-bcde-f12345678901 Small Container 2026-03-09T10:00:00Z
ID UID DISPLAY NAME CREATED

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documented catalog-item table includes an ID column, but catalog_item.go emits UID, DISPLAY NAME, and CREATED without ID. Please update the example to match the actual CLI output or change the formatter and add an output-contract test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and added test: 654d515 and 0ad2494

Expect(body["display_name"]).To(Equal("Small Container"))
spec, ok := body["spec"].(map[string]any)
Expect(ok).To(BeTrue(), "request body must include spec")
resources, ok := spec["resources"].([]any)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new test asserts exactly one resource, so a regression that drops additional resources or resource-specific values can still pass. Please add a two-resource case with distinct values.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
and the catalog item example in the README already defines two resources (app, db), so the test can mirror that

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the tests 0ad2494

Comment thread README.md Outdated
- path: memory.size
display_name: "Memory (GB)"
type: string
pattern: '^[1-9][0-9]*m?$'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current diff contains trailing whitespace on this line, so git diff --check fails. Please remove it before merge.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed 654d515

Assisted-By: Cursor AI

Signed-off-by: Jennifer Ubah <[email protected]>
Assisted-By: Cursor AI

Signed-off-by: Jennifer Ubah <[email protected]>
@jenniferubah

Copy link
Copy Markdown
Author

@jordigilh, @gciavarrini please can you take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants