(feat): Multi resource catalog item and instance#11
Conversation
jenniferubah
force-pushed
the
catalog/composite-resolution
branch
11 times, most recently
from
67d0d55 to
98af244
Compare
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
jenniferubah
force-pushed
the
catalog/composite-resolution
branch
from
98af244 to
88da51a
Compare
jenniferubah
changed the title
jenniferubah
requested review from
gabriel-farache,
gciavarrini and
machacekondra
as code owners
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
| } | ||
|
|
||
| // TODO: Rehydrate for multi-resources | ||
| oldResourceID := instance.Spec.ResourceIDs[0] |
There was a problem hiding this comment.
is it possible that Spec.ResourceIDs[0] is empty so this access would panic?
| 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
| WHERE json_extract(resource.value, '$.service_type') = ? | ||
| )`, serviceType) | ||
| default: | ||
| return query |
There was a problem hiding this comment.
so no filter is the DB is neither PSQL or sqlite? SHould we at least log a warning?
| return nil | ||
| } | ||
|
|
||
| func sameStringSlice(a, b []string) bool { |
There was a problem hiding this comment.
I think we can use slices.Equal from the std lib instead
| 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
3 participants
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.