kubectl operator list-operands library code #37

exdx · 2021-04-05T21:57:22Z

library code for listing custom resources on-cluster that can then be fed to a command or other callers

joelanford

Looks like a great start! I went through and did a first pass review, mainly with an eye toward consistency with kubectl user expectations and consistency with patterns used elsewhere in kubectl-operator.

internal/cmd/operator_list_custom_resources.go

internal/pkg/action/operator_list_custom_resources.go

internal/cmd/operator_list_custom_resources.go

internal/pkg/action/config.go

internal/pkg/action/operator_list_custom_resources.go

joelanford · 2021-04-07T01:34:02Z

internal/cmd/operator_list_custom_resources.go

+
+func bindOperatorListCustomResourcesFlags(fs *pflag.FlagSet, CRLister *action.OperatorListCustomResources) {
+	fs.BoolVarP(&CRLister.ListAll, "list-all", "y", true, "List all CRs in the result without batching. Defaults to true.")
+	fs.StringVarP(&CRLister.Namespace, "namespace", "n", "default", "Set namespace in which to look for operator.")


You misunderstood my first comment. You'll get --namespace for free. You can access it via config.Namespace. If --namespace is unset, it will be the value of the namespace from the context. If --namespace is set, it will be whatever it is set to.

After having talked about this more today, I think we explicitly don't want an --all-namespaces flag, because we're always looking for a specific operator in a specific namespace.

joelanford · 2021-04-07T01:45:09Z

May as well bump operator-framework/api to the latest and greatest. Looks like the one we're currently on is pretty old.

joelanford

My big concern is making the public API surface only as big as necessary. This PR is the first time we'll be making any of the Go code in this repo public, so I think we should be judicious.

So most of this review is my thoughts on doing that and giving other clients what they would need to list operands for a particular operator.

internal/pkg/action/operator_list_custom_resources.go

joelanford · 2021-04-07T15:27:44Z

pkg/action/custom_resource_lister.go

+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+


I would suggest:

Combining internal/pkg/action/operator_list_custom_resources.go and this file.

Putting the combined file at pkg/action/operator_list_operands.go so that it can be imported for external client's uses.

Unexporting everything except OperatorListOperands and its Run function.

Moving internal/pkg/action/config.go to pkg/action/config.go

pulling out the Configuration to its standalone file is producing a pretty big diff so right now I'm just going to import the existing struct and we can pull it out in another PR or commit

Sounds good. We just have to do that before external projects can use it because action.Configuration is needed when constructing the action struct.

internal/pkg/action/operator_list_custom_resources.go

pkg/action/custom_resource_lister.go

internal/cmd/operator_list_custom_resources.go

pkg/action/custom_resource_lister.go

pkg/action/operator_list_operands.go

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

exdx requested review from joelanford and benluddy April 5, 2021 21:57

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2021

exdx force-pushed the feat/list-operands branch 2 times, most recently from ee47b3e to 21d837a Compare April 5, 2021 22:10

joelanford reviewed Apr 6, 2021

View reviewed changes

internal/cmd/operator_list_custom_resources.go Outdated Show resolved Hide resolved

joelanford reviewed Apr 6, 2021

View reviewed changes

internal/pkg/action/config.go Outdated Show resolved Hide resolved

joelanford reviewed Apr 6, 2021

View reviewed changes

internal/pkg/action/operator_list_custom_resources.go Outdated Show resolved Hide resolved

exdx force-pushed the feat/list-operands branch from 21d837a to 361eeb9 Compare April 6, 2021 20:04

wip: list operands

0177fb8