kubernetes-sigs / cluster-addons

Addon operators for Kubernetes clusters.
Apache License 2.0
155 stars 47 forks source link

kubectl-ownerref: use kubectl helpers #102

Closed justinsb closed 3 years ago

justinsb commented 3 years ago

By reusing the kubectl helpers, we are able to get (relatively) easy yaml output etc, at the expense of a little boilerplate.

k8s-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-addons/blob/master/OWNERS)~~ [justinsb] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
atoato88 commented 3 years ago

I checked the output on k8s cluster w/ coredns-operator and I got the result as follows:

$ kubectl ownerref ownerref -A CoreDNS coredns-operator
NAMESPACE     NAME                     AGE
kube-system   serviceaccount/coredns   7m29s 

kube-system   serviceaccount/coredns-52c752mgc9   7m29s

kube-system   serviceaccount/kube-dns   7m29s

kube-system   serviceaccount/coredns   7m29s

      serviceaccount/system:coredns   3d

      serviceaccount/system:coredns   3d

coredns-operator creates some kinds which are Deployment, Service, ConfigMap, ... But output has serviceaccount/* kind only. I guess here is the cause point. The kind don't change once specific resourcePrinter is set (ex. serviceaccount). But you set it as cached var explicitly, so I think some updates are needed on current PR.

atoato88 commented 3 years ago

And current command line is like as follows:

$ kubectl ownerref ownerref -A CoreDNS coredns-operator    

Double ownerref are needed. Can we modify this to single ownerref only?

justinsb commented 3 years ago

Thanks for looking @atoato88 ... the kind problem in particular is tricky, because the TablePrinter seems to hack the kind in, and we need to cache it because otherwise yaml printing doesn't separate propertly.

Going to hold while I try to figure out how to fix the problems you identified - thanks again!

/hold

justinsb commented 3 years ago

Thinking about the kubectl ownerref ownerref problem, I'm wondering if a syntax like kubectl query owner(CoreDNS:coredns) or something of that nature might be more flexible. But I think we can always add that as an alternative command with the same "engine" here.

I've fixed this up now so it should address the problems you identified @atoato88 - thank you!

The printing is not what I would call perfect (yaml outputs an extra --- at the end, for example), but I think it gets us closer!

/hold cancel

justinsb commented 3 years ago

Thanks @atoato88 - great points; I incorporated your suggestions!

atoato88 commented 3 years ago

/lgtm Thank you to incorporate them.