-
Notifications
You must be signed in to change notification settings - Fork 531
Add Error Event for FederatedNamespace #1063
Add Error Event for FederatedNamespace #1063
Conversation
Welcome @qiujian16! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pitching in! Functionally looks fine, just some wording suggestions inline.
pkg/controller/sync/accessor.go
Outdated
// A FederatedNamespace is only valid for propagation | ||
// if it has the same name as the containing namespace. | ||
a.eventRecorder.Eventf( | ||
resource, corev1.EventTypeWarning, "InConsistentNameNamespace", "Name and namespace are not consistent") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/InConistentNameNamespace/InvalidName/
s/Name and namespace are not consistent/The name of a federated namespace must match the name of its containing namespace./
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@font Maybe federated resource validation, in addition to dry-run, could have special handling for federated namespaces to prevent creation when the name doesn't match that of the containing namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks for the comment.
128eeaa
to
61691fe
Compare
61691fe
to
b759fbd
Compare
pkg/controller/sync/accessor.go
Outdated
// A FederatedNamespace is only valid for propagation | ||
// if it has the same name as the containing namespace. | ||
a.eventRecorder.Eventf( | ||
resource, corev1.EventTypeWarning, "InvalidName", ""+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to include ""+
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right...fixed
b759fbd
to
b358242
Compare
Add an error event when name and namespace of federatednamespace are not same.
b358242
to
02d0085
Compare
Thank you! /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marun, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Add an error event when name and namespace of federatednamespace
are not same.
Which issue(s) this PR fixes:
Fixes #1037
Special notes for your reviewer:
Release note: