-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: carriedNamespace/carriesIgnoredNamespace account for Namespace object #1619
Conversation
Signed-off-by: Case Wylie <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1619 +/- ##
==========================================
+ Coverage 81.31% 81.40% +0.09%
==========================================
Files 43 43
Lines 1969 1979 +10
Branches 435 438 +3
==========================================
+ Hits 1601 1611 +10
Misses 340 340
Partials 28 28
|
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.
Wait, but... Namespace objects don't "carry" namespaces -- they have no metadata.namespace
prop, nor should they -- because they "are" namespaces. Namespaces shouldn't carry namespace
props, right? So why would we want this method to present like they do?
Also, this edit makes the carriedNamespace()
give back what carriedName()
already gives back but only for Namespaces which is confusing, IMO. Why was this update needed, exactly?
The reason this is need is due to the fact that if you are using When(a.Namespace)
.IsCreated()
.WithName("pepr-demo-2")
.Watch(async ns => {
Log.info("Namespace pepr-demo-2 was created."); RE- |
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.
Seems like the uncarryableNamespace() func would be a better place to put the new check -- this method lives one-level above the carriesNamespace
and carriedNamespace
methods & is the actual method responsible for using available information to give a final determination on whether a forbidden namespace is in use or not.
If we want to update our definition of what it means to "use a forbidden namespace" from only "has a forbidden metadata.namespace defined" and have it also include "or IS a Namespace with a forbidden name" then I think that would be the place.
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
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.
Yep, seems like that should do it.
Description
CarriedNamespace did not account for Namespace objects which caused some strange behaviors in #1610
After Fix:
Related Issue
Fixes #1618
Relates to #
Type of change
Checklist before merging