Conversation
Signed-off-by: Wassim DHIF <wassim.dhif@datadoghq.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Wassim DHIF <wassim.dhif@datadoghq.com>
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 <wassim.dhif@datadoghq.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Wassim DHIF <wassim.dhif@datadoghq.com>
Signed-off-by: Wassim DHIF <wassim.dhif@datadoghq.com>
🛑 Gate Violations
ℹ️ Info🎯 Code Coverage (details) 🔗 Commit SHA: dbba436 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (46.34%) 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 #2910 +/- ##
==========================================
+ Coverage 40.06% 40.08% +0.01%
==========================================
Files 320 320
Lines 28086 28075 -11
==========================================
Hits 11254 11254
+ Misses 16022 16012 -10
+ Partials 810 809 -1
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:
|
…eneric-resource-improvements
…/generic-resource-improvements-context-logging
tbavelier
changed the title
…/generic-resource-improvements-context-logging
…/generic-resource-improvements-context-logging
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55a8e431b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
tbavelier
changed the title
tbavelier
changed the title
gabedos
left a comment
gabedos
left a comment
There was a problem hiding this comment.
Updating logging + Reconcile tweak lgtm
tbavelier
deleted the
tbavelier/generic-resource-improvements-context-logging
branch
4 participants
What does this PR do?
Transitions the GenericResource controller to context-based logging, consistent with the pattern applied to other controllers (e.g.
datadogagentinternal). Also eliminates the doubleclient.Getper reconcile that was mistakingly introduced at one pointContext-based logging
Reconcilemethod indatadoggenericresource_controller.goviactrl.LoggerInto, and flows down through the contextupdate,create,updateStatusIfNeeded,handleFinalizer, etc.) now takectx context.Contextinstead oflogger logr.Logger— same parameter count, but a strict improvement:ctxreplacescontext.TODO()in client calls with the real request context (correct deadline/cancellation propagation), and the logger retrieved viactrl.LoggerFrom(ctx)automatically carries the request-scoped key/values<resources>.gosince it would be twice the same information, so we can simplify:{ "level": "INFO", "ts": "2026-04-17T08:10:44.469Z", "logger": "controllers.DatadogGenericResource", "msg": "created a new dashboard", "namespace": "system", "name": "ddgr-test-dashboard", "reconcileID": "9c2e29ca-8f63-4674-81c2-07a0dad4c9bd", "dashboard Id": "shi-zuy-uz7" } { "level": "INFO", "ts": "2026-04-17T08:10:44.470Z", "logger": "controllers.DatadogGenericResource", "msg": "created a new DatadogGenericResource", "namespace": "system", "name": "ddgr-test-dashboard", "reconcileID": "9c2e29ca-8f63-4674-81c2-07a0dad4c9bd", "generic resource Id": "shi-zuy-uz7" }Eliminate double
client.Getreconcile.AsReconciler(used inSetupWithManager) already fetches the object and handles not-found before calling our reconciler. The old internalReconcile(ctx, request)was doing a secondclient.Geton top of that.The fix aligns with the pattern used by
datadogmonitor,datadogagent, and other controllers: the internal reconciler'sReconcileaccepts*DatadogGenericResourcedirectly (no fetch inside), and the framework-owned fetch inreconcile.AsReconcileris the only one. The now-redundantReconcileInstanceindirection is removed.Motivation
Consistency with the rest of the codebase. Using
ctrl.LoggerFrom(ctx)is the idiomatic controller-runtime pattern and avoids threading a logger explicitly through every call. It also fixes the pre-existing issue of client operations usingcontext.TODO()instead of the real request context.Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel