Conversation
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
dc756c7 to
c6c924f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new CloudProfile spec JSON logging can leak provider-specific configuration and does unnecessary work even when verbose logging is disabled, so it should be guarded/redacted and error-aware before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds additional logging to the controller reconciliation flow to improve observability when reconciling ManagedCloudProfile resources and applying the generated Gardener CloudProfile.
Changes:
- Log reconcile triggers with object generation/resourceVersion and log when status patches are skipped/issued.
- Log the
CreateOrPatchoperation result for the generatedCloudProfileand (on updates) emit a spec “before/after” debug log.
File summaries
| File | Description |
|---|---|
| controllers/managedcloudprofile_controller.go | Adds verbose reconcile/status-patch debug logs to improve traceability of reconciliation progress. |
| controllers/cloud_profile.go | Adds logging around CreateOrPatch operations and emits a spec “diff” log for updated CloudProfiles. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Aliaksei Dziauho <a.dziauho@sap.com>
8f37e26 to
797770f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new spec-diff logging can expose large and potentially sensitive provider configuration at runtime and should be adjusted/redacted before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
controllers/cloud_profile.go:66
CloudProfile patch operationis logged at verbosity 0, so it will show up on every reconcile (even when nothing changes), which can be noisy for a controller that requeues periodically. Consider making this a debug-level log (V(1)) so it’s only emitted when higher verbosity is enabled.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| if op == controllerutil.OperationResultUpdated && log.V(1).Enabled() { | ||
| beforeJSON, err := json.Marshal(specBefore) | ||
| if err != nil { | ||
| log.Error(err, "failed to marshal spec before") | ||
| } | ||
| afterJSON, err := json.Marshal(cloudProfile.Spec) | ||
| if err != nil { | ||
| log.Error(err, "failed to marshal spec after") | ||
| } | ||
| log.V(1).Info("CloudProfile spec diff", "before", string(beforeJSON), "after", string(afterJSON)) | ||
| } |
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
There was a problem hiding this comment.
🟡 Changes recommended
Current CloudProfile logging is noisy/misordered (logs before error handling) and includes potentially sensitive/large spec dumps at low verbosity, plus a misleading “skipping retry” message.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
controllers/cloud_profile.go:70
CloudProfile patch operationis logged at info level and the spec before/after log runs before theerr != nilhandling. This can add noisy logs and also prints fullCloudProfileSpecJSON (which may include large/sensitive fields likecaBundle/providerConfig) at V(1). Consider only logging after a successful CreateOrPatch, lowering to V(1) for the operation, and moving the full spec dump behind a higher verbosity (and only when marshaling succeeds).
log.Info("CloudProfile patch operation", "operation", op)
if op == controllerutil.OperationResultUpdated && log.V(1).Enabled() {
beforeJSON, err := json.Marshal(specBefore)
if err != nil {
log.Error(err, "failed to marshal spec before")
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| return fmt.Errorf("failed to patch ManagedCloudProfile status: %w", statusErr) | ||
| } | ||
| if apierrors.IsInvalid(err) { | ||
| log.Error(err, "CloudProfile is invalid, skipping retry") |
No description provided.