-
Notifications
You must be signed in to change notification settings - Fork 196
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
Add owner-name label on resources #3608
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3608 +/- ##
==========================================
+ Coverage 53.08% 53.27% +0.19%
==========================================
Files 1286 1340 +54
Lines 406699 438163 +31464
==========================================
+ Hits 215894 233431 +17537
- Misses 159804 170240 +10436
- Partials 31001 34492 +3491 ☔ View full report in Codecov by Sentry. |
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.
Users can't filter w/ annotations I don't think, only with labels?
So I think this needs to be a label not an annotation.
|
||
const ( | ||
PerResourceSecret = "serviceoperator.azure.com/credential-from" | ||
OwnedByAnnotation = "serviceoperator.azure.com/owned-by" |
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.
I think owner-name
or owner
would be clearer than owned-by
.
Do we also need to think about owner-group-kind
or something as well, for extension resources such as roleassignment?
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 makes sense - Users can filter through annotations but isn't as easy as with labels.
Made the changes to add label instead of annotation and changes the name to owned-by
to owner-name
.
Not very sure about if adding owner-group-kind
would bring much value.. since user can anyway filter based on owner-name - which is more specific rather than filtering all resources based on just group + kind?
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.
Not having owner-group-kind
forces the user to do another query to discover the owner type if they don't already know it. I think there's value in making it easy/obvious.
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.
Sure! and that applies to only resources which are KubernetesReferences?
Closes #3190
What this PR does / why we need it:
This PR enables controller to add
serviceoperator.azure.com/owner-name: <OwnerName>
label at the time of adding the initial resource state