Fix unknown fields warnings and possibility to suppress finalizer ones #786
Conversation
Signed-off-by: Hasan Turken <[email protected]>
| @@ -63,21 +64,6 @@ type Unstructured struct { | |||
| unstructured.Unstructured | |||
| } | |||
|
|
|||
| // Reference to a claim. | |||
| type Reference struct { | |||
There was a problem hiding this comment.
If I simply added a Reference type under composite package, I was getting import cycle. So, I moved them into a separate reference package.
turkenh
force-pushed
the
fix-unknown-fields
branch
2 times, most recently
from
5e5b686
to
a34fc1d
Compare
| if err := fieldpath.Pave(c.Object).GetValueInto("spec.compositionRevisionRef", out); err != nil { | ||
| return nil | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| // SetCompositionRevisionReference of this resource claim. | ||
| func (c *Unstructured) SetCompositionRevisionReference(ref *corev1.ObjectReference) { | ||
| func (c *Unstructured) SetCompositionRevisionReference(ref *corev1.LocalObjectReference) { |
There was a problem hiding this comment.
| if err := fieldpath.Pave(c.Object).GetValueInto("spec.resourceRef", out); err != nil { | ||
| return nil | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| // SetResourceReference of this composite resource claim. | ||
| func (c *Unstructured) SetResourceReference(ref *corev1.ObjectReference) { | ||
| func (c *Unstructured) SetResourceReference(ref *reference.Composite) { |
There was a problem hiding this comment.
| func (c *Unstructured) GetCompositionRevisionReference() *corev1.ObjectReference { | ||
| out := &corev1.ObjectReference{} | ||
| func (c *Unstructured) GetCompositionRevisionReference() *corev1.LocalObjectReference { | ||
| out := &corev1.LocalObjectReference{} |
There was a problem hiding this comment.
turkenh
changed the title
turkenh
changed the title
turkenh
changed the title
pkg/resource/fake/mocks.go
Outdated
|
|
||
| // ManagedResourceReferencer is a mock that implements ManagedResourceReferencer interface. | ||
| type ManagedResourceReferencer struct{ Ref *corev1.ObjectReference } | ||
| type ManagedResourceReferencer struct{ Ref *reference.Composite } |
There was a problem hiding this comment.
are these updates to ManagedResourceReferencer correct? Because it is called ManagedResource*, I would expect them not to only specifically point to a Composite now 🤔
There was a problem hiding this comment.
Great catch! Fixing linter issues one by one and not sure how/why I changed this one 🤔
pkg/warning/warning.go
Outdated
| // accidental conflicts with other finalizer writers. | ||
| func DomainQualifiedFinalizer(domain string) regexp.Regexp { | ||
| return *regexp.MustCompile( | ||
| fmt.Sprintf("^metadata.finalizers:.*%s.*prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers$", domain), |
There was a problem hiding this comment.
will this check start to fail once we pick up https://github.com/kubernetes/kubernetes/pull/127303/files which changes the warning message?
There was a problem hiding this comment.
Yes, correct. Logs would appear again until we adapt the regexp.
I'll drop the finalizer log suppressing part.
pkg/warning/warning.go
Outdated
| */ | ||
|
|
||
| // Package warning contains utilities for handling warnings. | ||
| package warning |
There was a problem hiding this comment.
i'm wondering how essential this is actually - it doesn't fix the underlying issue right, it just hides it from the logs? i'm wondering if we just live with the warnings in the logs now, and resolve them with a real fix during the 1.19 milestone, instead of hiding them for now and maybe forgetting about them. What do you think?
There was a problem hiding this comment.
I'll drop the last commit, e.g., finalizer log suppressing, since both you and @phisco prefer keeping logs around instead, and I am not too opinionated, at least to consider something that important doing last minute :)
Signed-off-by: Hasan Turken <[email protected]>
turkenh
force-pushed
the
fix-unknown-fields
branch
from
1f25008
to
70499a4
Compare
|
Successfully created backport PR for |
Description of your changes
This PR aims to clean up warnings observed in crossplane/crossplane#6043 by:
Suppressing "prefer a domain-qualified finalizer" until we properly migrate them to the suggested format.Together with the accompanying PR in Crossplane, I could validate that former warnings disappeared with the changes here.
This PR introduces a new package,warning, with thewarning.LoggingHandlerimplementing rest.WarningHandler interface. This supports deduplication just like the current handler Crossplane uses, but it also uses provided crossplane runtime logger so that warnings logged as structured logs* and also enables discarding logs matching provided regular expressions. I opted to put this into crossplane runtime since we may want to leverage it in other repositories, e.g. providers, as well.I have:
earthly +reviewableto ensure this PR is ready for review.Linked a PR or a docs tracking issue to document this change.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.