Conversation
…urces Fix 1 (provider.go): Change DOProvider.Apply second-pass deferred flush to iterate p.drivers directly instead of plan.Actions. This ensures orphaned deferred entries (from a prior transient flush failure) are flushed even when the resource type no longer appears in the current plan. Fix 2 (database.go): Modify buildCreateFirewallRulesExcludingApps and buildUpdateFirewallRulesExcludingApps to pass through UUID-shaped type=app values directly (not defer them), using the existing isUUIDLike helper. Only slug-style app names require deferred resolution via Apps API. Add tests: - TestDOProvider_Apply_FlushesDeferred_WhenTypeAbsentFromPlan: validates orphan handling — flush runs even when resource type absent from plan - TestDatabaseDriver_Create_AppUUID_PassedThroughNotDeferred: UUID app entries pass through to create request; slug entries still deferred - TestDatabaseDriver_Update_AppUUID_PassedThroughNotDeferred: UUID app entries pass through to partial update; slug entries still deferred Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-digitalocean/sessions/a19c1855-ccdd-42c7-8255-80a2d7a79605 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR delivers two correctness fixes for the DatabaseDriver deferred trusted_sources flush behavior introduced previously: (1) ensure deferred queues can’t become permanently orphaned when a resource type disappears from later plans, and (2) avoid over-deferring type=app trusted sources when the value is already a UUID.
Changes:
- Flush deferred updates by iterating the provider’s driver registry (
p.drivers) rather thanplan.Actions, ensuring orphaned deferred entries are still retried. - Allow
type=apptrusted_sources entries with UUID-shaped values to pass through immediately (only slug-style app names are deferred). - Add regression tests for orphan flush when the resource type is absent from the plan, and for UUID passthrough on both Create and Update deferred paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/provider.go | Flush deferred updates by scanning all registered drivers instead of current plan actions. |
| internal/provider_deferred_test.go | Adds a provider-level regression test for flushing orphaned deferred entries when no actions exist for that type. |
| internal/drivers/database.go | Updates partial-rule builders to only exclude non-UUID type=app entries, enabling UUID passthrough. |
| internal/drivers/database_deferred_test.go | Adds regression tests ensuring UUID type=app entries are not deferred on Create/Update partial application. |
Comments suppressed due to low confidence (1)
internal/provider.go:299
- Iterating over
p.drivers(a Go map) makes the deferred-flush order and any appendedresult.Errorsordering non-deterministic across runs. If multiple drivers have deferred updates, this can cause noisy/flaky output comparisons. Consider collecting driver keys, sorting them, then flushing in a stable order.
for resourceType, d := range p.drivers {
du, ok := d.(deferredUpdater)
if !ok || !du.HasDeferredUpdates() {
continue
}
if flushErr := du.FlushDeferredUpdates(ctx); flushErr != nil {
result.Errors = append(result.Errors, interfaces.ActionError{
Resource: resourceType,
Action: "deferred_update",
Error: flushErr.Error(),
})
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The failure is in the |
Two follow-up correctness fixes to the
DatabaseDriverdeferredtrusted_sourcesflush queue introduced in PR #44.Fix 1 — Orphan deferred entries when resource type leaves the plan
DOProvider.Apply's second-pass flush iteratedplan.Actionsto find drivers to flush. If a flush failed transiently and the next plan omitted that resource type entirely, the deferred queue entries were permanently stranded.Fix: iterate
p.drivers(the registry) directly — every driver withHasDeferredUpdates() == trueis flushed unconditionally, regardless of whether its type appears in the current plan.Fix 2 — UUID-shaped type=app values over-deferred
buildCreateFirewallRulesExcludingApps/buildUpdateFirewallRulesExcludingAppsexcluded alltype=appentries unconditionally, deferring even entries whose value is already a resolved UUID (no Apps API lookup needed).Fix: only exclude
type=appentries whose value failsisUUIDLike(). UUID-shaped values pass through to the create/update call immediately; only slug-style names are deferred.New regression tests cover both paths: orphan flush with type-absent plan, and UUID passthrough on both the create and update deferred paths.