(catalog): Multi resource catalog item and instance#11
Conversation
67d0d55 to
98af244
Compare
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
98af244 to
88da51a
Compare
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
| inst, err := s.Get(ctx, id) | ||
| if err != nil { | ||
| if errors.Is(err, ErrCatalogItemInstanceNotFound) { | ||
| return nil, ErrCatalogItemInstanceNotFound | ||
| } | ||
| return nil, fmt.Errorf("failed to get catalog item instance: %w", err) | ||
| } | ||
| if len(inst.Spec.ResourceIDs) == 0 || inst.Spec.ResourceIDs[0] != expectedResourceID { | ||
| return nil, ErrCatalogItemInstanceConflict | ||
| } | ||
|
|
||
| inst.Spec.ResourceIDs[0] = newResourceID | ||
| result := s.db.WithContext(ctx).Model(&inst).Where("id = ?", id).Select("spec").Updates(&inst) |
There was a problem hiding this comment.
the DB entry should be locked during this to avoid having potential race here where 2 requests race each other: we get the data from the DB, then some checks are performed on the app and only then the entry is updated in the DB. So during the time the checks are performed, some other changes may be applied to the DB.
So we should either block any change (or at least update) to be applied to the resource or do the update in 1 call as it was done previously
There was a problem hiding this comment.
So the plan is to will be refactor this function in a follow up rehydration PR, hence the TODO. And I agree we should lock the transaction to prevent any race condition. Will do that in as part of the rehydration PR as well.
| if err := validateUserValuesForCatalogItem(catalogItem.Spec, req.Spec.UserValues); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
this is called by BuildResourceGraph which is called by createInstance so I guess this call is duplicated
| if !ok { | ||
| return false | ||
| } | ||
| return strings.Contains(str, "${") |
There was a problem hiding this comment.
this is to broad IMO. We should match for the whole pattern that we defined celReferencePattern
There was a problem hiding this comment.
This check was used to fail any CEL expression in the user values but we don't want that so removed it. Updated it now to allow CEL in editable user values and validate it. See 2f3e26e.
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
| // Call Placement Manager with the first resource | ||
| // until multi-resource placement deletion is wired. | ||
| s.logger.DebugContext(ctx, "Calling placement manager to delete resource", "id", id, "resource_id", instance.Spec.ResourceIDs[0]) | ||
| if err := s.pmClient.DeleteResource(ctx, instance.Spec.ResourceIDs[0]); err != nil { |
There was a problem hiding this comment.
Indexing [0] here can panic on empty rows
There was a problem hiding this comment.
Yes, I agree. It is only a placeholder since multi resource placement is not implemented yet. Will update this entirely when we have it.
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
## Summary - Update `initRehydrationCatalogItem()` creation payload to use the new `spec.resources[]` schema introduced by [dcm-project/control-plane#11](dcm-project/control-plane#11) - Update listing logic to find `service_type` inside `spec.resources[]` (with fallback to legacy flat format) - Add `resource` field to `UserValue` in `createTestInstance()` and `defaultUserValues()` ## Problem Since Jul 3 (control-plane PR #11 merge), the `BeforeSuite` in E2E tests fails with: ``` catalog item creation returned unexpected status 400 ``` This causes **all 145 tests to be skipped** on every nightly run (builds 170–194+). The root cause is the catalog item API now requires `spec.resources[{name, service_type, fields}]` instead of the old flat `spec.service_type + spec.fields`. The OpenAPI validation middleware rejects the old payload. ## Test plan - [x] Trigger `CI/flightpath-dcm-deploy` job with this branch to verify BeforeSuite passes - [x] Confirm rehydration tests run (not skipped) and pass Made with [Cursor](https://cursor.com) Signed-off-by: Vladislav Kolodny <vkolodny@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…ma (#28) ## Summary - Update test fixtures in `catalog_item_test.go` to use the new multi-resource catalog item schema (`spec.resources[]` instead of flat `spec.service_type`) - Fix `catalogItemTableDef` RowFunc in `catalog_item.go` to extract `service_type` from `resources[0]` so the SERVICE TYPE table column renders correctly against the updated control-plane Relates to: FLPATH-4617 Depends on control-plane PR #11 (dcm-project/control-plane#11) which introduced the multi-resource catalog item schema. ## User Impact Users will now see catalog items in the CLI correctly when the control-plane uses the new multi-resource schema, with service type information displayed properly in table output. Signed-off-by: Chad Crum <1462069+chadcrum@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Add catalog item and and catalog instance resolution for multiple resource within a single request
Enhancement: dcm-project/enhancements#55
Note: Creation and Deletion for multi resource placement and rehydration are out of scope for this PR. These will be in a follow up PR and are marked as TODO in this PR.