-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
This will break history support, so I'll revert this commit.
This reverts commit 31d3bcd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -299,8 +286,8 @@ func (e TableExecutor) resolveResources(ctx context.Context, meta schema.ClientM | |||
diags diag.Diagnostics | |||
) | |||
|
|||
for _, o := range objects { | |||
resource := schema.NewResourceData(e.Db.Dialect(), e.Table, parent, o, e.metadata, e.executionStart) | |||
for i := range objects { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unnecessary, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the interface{}
isn't a pointer then this change is necessary (and more future proof) due to the way for
for range
works in go.
if err := e.cleanupStaleData(ctx, client, parent); err != nil { | ||
return nc, diags.Add(fromError(err, diag.WithType(diag.DATABASE), diag.WithSummary("failed to cleanup stale data on table %q", e.Table.Name))) | ||
|
||
if !diags.HasErrors() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a strong gut feeling we should avoid this change for now:
- Even a single column (e.g. in a child relation) that failed to be resolved can cause "error" diags, right? So a single column with a null value in any child relation will cause failure of cleanup .
Seems dangerous in any case. I'd really avoid doing this unless we have a specific reason to do it (e.g. customers complining, sentry errors, ....).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the column resolver returns error diags, it doesn't continue to complete all other tasks so (if there are any) the new rows are incomplete anyway. no postresourceresolver called, no internal resolvers (cq_id!) called. https://github.com/cloudquery/cq-provider-sdk/blob/main/provider/execution/execution.go#L367-L370
not sure if it will lead to more rows or not on edge cases.
Various fixes
TableExecutor
inwith*()
functionsMore to come:
ExtraFields
(sdk / core / cqproto) and all support for additional deletefilter columns etc. (this will break history so I reverted -- later in separate PR)AlwaysDelete
? (not used anywhere unless there's a private provider I'm not aware of)CopyFrom
part disabled, the delete doesn't remove anything (and no on conflict) so it can't run more than once (fixed in fix: Delete by cq_id before insertion #266)