-
Notifications
You must be signed in to change notification settings - Fork 213
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
Delete controllers package #270
Conversation
Robert Brennan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
pkg/kube/workload.go
Outdated
func NewGenericWorkload(originalResource kubeAPICoreV1.Pod, dynamicClientPointer *dynamic.Interface, restMapperPointer *meta.RESTMapper) GenericWorkload { | ||
workload := GenericWorkload{} | ||
workload.Name = originalResource.Name | ||
workload.Namespace = originalResource.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.
@baderbuddy I can't figure out why originalResource.Name
is defined, based on the godoc
Looks like we're tracking Name
and Namespace
in addition to ObjectMeta
- any thoughts on getting rid of one or the other?
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.
Yeah using ObjectMeta to get at Name and Namespace makes sense. I'm pretty sure the .Name
and .Namespace
are just shorthand.
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.
Ohh I misunderstood your question. Ooh that's a great point. Right now we keep the Pod's Meta no matter what, but the Name matches the top level controller. I could see value in the metadata especially for Annotations, but for the most part I think just the name and Namespace are all we really need.
Codecov Report
@@ Coverage Diff @@
## master #270 +/- ##
==========================================
- Coverage 55.64% 55.29% -0.35%
==========================================
Files 12 12
Lines 629 604 -25
==========================================
- Hits 350 334 -16
+ Misses 247 237 -10
- Partials 32 33 +1
Continue to review full report at Codecov.
|
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.
This looks so much cleaner. I like it!
@@ -72,13 +72,6 @@ customChecks: | |||
not: | |||
pattern: ^quay.io | |||
|
|||
controllersToScan: |
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.
Do we want to leave this to limit the validating webhook?
pkg/kube/workload.go
Outdated
func NewGenericWorkload(originalResource kubeAPICoreV1.Pod, dynamicClientPointer *dynamic.Interface, restMapperPointer *meta.RESTMapper) GenericWorkload { | ||
workload := GenericWorkload{} | ||
workload.Name = originalResource.Name | ||
workload.Namespace = originalResource.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.
Ohh I misunderstood your question. Ooh that's a great point. Right now we keep the Pod's Meta no matter what, but the Name matches the top level controller. I could see value in the metadata especially for Annotations, but for the most part I think just the name and Namespace are all we really need.
No description provided.