fix(normalizers): include resource context in failed normalization log#27769
Open
rafaelmfried wants to merge 1 commit intoargoproj:masterfrom
Open
fix(normalizers): include resource context in failed normalization log#27769rafaelmfried wants to merge 1 commit intoargoproj:masterfrom
rafaelmfried wants to merge 1 commit intoargoproj:masterfrom
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
The "Failed to apply normalization" debug message previously logged only the underlying error, leaving operators with no way to identify which resource the patch was targeting. With cluster-wide ignoreDifferences rules, this can fire dozens of times per reconcile loop with no signal about the offending resource. Add the resource's group, kind, name, and namespace as structured fields on the existing Debugf call so the log entry is actionable. The shouldLogError filter is left intact — it already silences the most common "nonexistent key" / "doc is missing path" cases that triggered the original report. The normalizerPatch interface is intentionally not extended to expose the JSON-patch path itself; the resource context already addresses the reporter's "some portion of docData" ask without expanding scope. Closes argoproj#14148 Signed-off-by: Rafael Moura Friederick <rafael.mf.documents@gmail.com>
9c85370 to
410e2b6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #27769 +/- ##
=======================================
Coverage 63.96% 63.97%
=======================================
Files 421 421
Lines 57774 57780 +6
=======================================
+ Hits 36954 36963 +9
+ Misses 17339 17337 -2
+ Partials 3481 3480 -1 ☔ View full report in Codecov by Sentry. |
Member
|
Seems reasonable to me. |
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
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.
Closes #14148.
What
Adds the resource's
group,kind,name, andnamespaceas structured fields on theFailed to apply normalizationdebug log inutil/argo/normalizers/diff_normalizer.go.Why
The original message gave operators no way to identify which resource the failing patch was targeting. With cluster-wide
ignoreDifferencesrules this can fire dozens of times per reconcile loop, producing noise that's hard to act on.Reporter's exact pain in the issue:
Diff
Scope decisions (open to changing in review)
shouldLogErrorfilterUnable to remove nonexistent key,remove operation does not apply: doc is missing path) that triggered the original report.normalizerPatchinterface (line 27) — disproportionate to the bug. Resource context alone already answers the reporter's "some portion ofdocData" ask. Happy to follow up in a separate PR if the team wants the path too.Failed to apply normalization patch(kept "Failed", added "patch" for precision)shouldLogErrorfiltering, what reaches this branch is a real failure to apply the patch. Open toSkipped normalization patchif you prefer a less alarming tone.Test
TestNormalizeFailureLogIncludesResourceContextindiff_normalizer_test.go— reuses the existing helpful-error trigger pattern fromTestJQPathExpressionReturnsHelpfulErrorand asserts viatest.CaptureLogEntriesthatkind=ConfigMapandname=my-configmapappear in the captured output along with the new message.Full package suite (
go test ./util/argo/normalizers/...) is green locally — 15/15.Notes
Failed to apply normalizationwarning #14148) before opening —@colecschmidthad previously expressed interest but no PR landed in 11 days, so I picked it up after pinging the issue.