[CONTP-1606] [DDGR] Reduce boilerplate: CreateResult, stateful handlers, handler registry#2911
Merged
tbavelier merged 40 commits intoApr 22, 2026
Merged
Conversation
Signed-off-by: Wassim DHIF <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Wassim DHIF <[email protected]>
MockHandler and its state (mockGetErr, mockUpdateErr, etc.) lived in utils.go but were only referenced from test files, causing golangci-lint to flag them as unused (and the error vars as violating errname). Moved all mock code to mock_handler_test.go and registered the handler via init(), with a testHandlers map in utils.go as the bridge.
Signed-off-by: Wassim DHIF <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Wassim DHIF <[email protected]>
Signed-off-by: Wassim DHIF <[email protected]>
…eneric-resource-improvements
…/generic-resource-improvements-context-logging
tbavelier
changed the title
…/generic-resource-improvements-context-logging
🛑 Gate Violations
ℹ️ Info🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 7d596f5 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (32.46%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2911 +/- ##
==========================================
- Coverage 40.08% 40.01% -0.08%
==========================================
Files 320 320
Lines 28075 28037 -38
==========================================
- Hits 11254 11219 -35
+ Misses 16012 16007 -5
- Partials 809 811 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…/generic-resource-improvements-context-logging
…ating status
Each ResourceHandler implementation previously owned error logging,
status mutation (Id, Created, Creator, SyncStatus, CurrentHash), and
had a fat signature inconsistent with the other three interface methods.
Introduce CreateResult{ID, CreatedTime, Creator} and reduce
createResourcefunc to (r, instance) -> (CreateResult, error), matching
get/update/delete. All shared logic (error logging, updateErrStatus,
status population) is consolidated in apiCreateAndUpdateStatus.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
tbavelier
force-pushed
the
tbavelier/generic-resource-improvements-create-result
branch
from
April 17, 2026 08:34
e2e84bf to
48d50bc
Compare
reconcile.AsReconciler already fetches the object before calling Reconcile(ctx, instance). The previous implementation rebuilt a Request and delegated to the internal Reconcile which performed a second Get, resulting in two cache reads per reconcile loop. Split internalReconcile into: - Reconcile(ctx, req): keeps the Get for backward-compat/test use - ReconcileInstance(ctx, instance): skips the Get, used by the outer controller which receives a pre-fetched object from AsReconciler Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…' into tbavelier/generic-resource-improvements-create-result
…' into tbavelier/generic-resource-improvements-create-result
Replace the switch statement and separate testHandlers map with a single handlers map. Adding a new resource type is now one map entry instead of a switch case. Tests register mocks directly into the same map. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Resolve the ResourceHandler once at the top of Reconcile and pass it through to create/update. Inline the handler calls in the finalizer. Remove the apiGet/apiUpdate/apiDelete/apiCreateAndUpdateStatus wrappers that were one-liner indirections. Replace thin Test_api* tests with a focused Test_getHandler that covers the registry lookup and panic path. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
monitors, notebooks, and synthetics were silently ignoring unmarshal errors, while dashboards and downtimes already checked them. Now all handlers surface invalid JSON specs as errors instead of sending zero-value bodies to the API. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Each ResourceHandler now holds its own auth context and typed API client, removing the coupling to the Reconciler struct. The Reconciler no longer carries 5 typed client fields — it holds a handlers map built by buildHandlers(). UpdateDatadogClient replaces the entire map on credential refresh. The ResourceHandler interface methods drop the *Reconciler parameter and are renamed from createResourcefunc/getResourcefunc/etc to createResource/getResource/etc. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
tbavelier
changed the title
…' into tbavelier/generic-resource-improvements-create-result
Base automatically changed from
tbavelier/generic-resource-improvements-context-logging
to
main
April 20, 2026 09:04
…te-result # Conflicts: # internal/controller/datadoggenericresource/controller.go # internal/controller/datadoggenericresource/dashboards.go # internal/controller/datadoggenericresource/downtimes.go # internal/controller/datadoggenericresource/mock_handler_test.go # internal/controller/datadoggenericresource/monitors.go # internal/controller/datadoggenericresource/notebooks.go # internal/controller/datadoggenericresource/resource_handler.go # internal/controller/datadoggenericresource/synthetics.go # internal/controller/datadoggenericresource/synthetics_test.go # internal/controller/datadoggenericresource/utils.go # internal/controller/datadoggenericresource/utils_test.go
git-thuerk-done
approved these changes
Apr 20, 2026
levan-m
approved these changes
Apr 21, 2026
tbavelier
deleted the
tbavelier/generic-resource-improvements-create-result
branch
April 22, 2026 06:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
1. Uniform
createResourcefunc: returnCreateResultinstead of mutating status (48d50bc)Makes
createResourcefuncconsistent with the other threeResourceHandlerinterface methods (get,update,delete).Previously,
createResourcefunchad a fat, inconsistent signature — it tookctx,status,now, andhashin addition torandinstance, and each of the 6 handler implementations owned error logging,updateErrStatuscalls, and direct status mutation.Introduces
CreateResult{ID, CreatedTime, Creator}and reduces the interface method to:All shared logic (error logging,
updateErrStatus, populatingId/Created/LastForceSyncTime/Creator/SyncStatus/CurrentHash) is now consolidated inr.create.2. Replace
getHandlerswitch with handler registry map (ab2b909)Replaces the switch statement and separate
testHandlersmap with a singlehandlersmap. Adding a new resource type is now one map entry instead of a switch case.3. Resolve handler once at Reconcile entry, remove wrapper layer (4d3de07)
Resolves the
ResourceHandleronce at the top ofReconcileand passes it through tocreate/update. Removes theapiGet/apiUpdate/apiDelete/apiCreateAndUpdateStatusone-liner wrapper functions and ther.getmethod. The handler is called directly at each call site.4. Fix inconsistent
json.Unmarshalerror handling (c2f623e)monitors.go,notebooks.go, andsynthetics.gowere silently ignoringjson.Unmarshalerrors, whiledashboards.goanddowntimes.goalready checked them. Now all handlers surface invalid JSON specs as errors.5. Make handlers stateful with own API clients (02c7d90)
Each
ResourceHandlernow holds its own auth context and typed API client (DashboardHandler{auth, client}instead ofDashboardHandler{}), removing the coupling to theReconcilerstruct. TheReconcilerno longer carries 5 typed client fields — it holds ahandlersmap built bybuildHandlers().UpdateDatadogClientreplaces the entire map on credential refresh.The
ResourceHandlerinterface methods drop the*Reconcilerparameter and are renamed fromcreateResourcefunc/getResourcefunc/etc tocreateResource/getResource/etc.Motivation
Reconcilerstruct was a bag of typed clients that every handler reached into — now each handler is self-containedHow to QA
Verify CRUD works: create a DDGR, update it, delete it, and verify at every step it's reflected in the UI
Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel