Fix unknown fields warnings and possibility to suppress finalizer ones#786
Fix unknown fields warnings and possibility to suppress finalizer ones#786turkenh merged 2 commits intocrossplane:mainfrom
Conversation
Signed-off-by: Hasan Turken <turkenh@gmail.com>
| } | ||
|
|
||
| // 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
|
|
||
| // 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.
|
|
||
| // 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
phisco
left a comment
phisco
left a comment
There was a problem hiding this comment.
not a huge fan of suppressing warnings, I would have probably preferred to keep them there and fix them in a follow-up PR.
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 <turkenh@gmail.com>
turkenh
force-pushed
the
fix-unknown-fields
branch
from
1f25008 to
70499a4
Compare
|
Successfully created backport PR for |
3 participants
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.