cvo/internal: catch up the operator status handler to PR #31

[abhinavdahiya]

/cc @wking

[wking]

Do we really need this? OperatorStatus objects still exist (as I understand it, e.g. see here). And OperatorStatus seems appropriate for registering newOperatorStatusBuilder. Maybe we need additional logic to register a builder for ClusterOperator?

[abhinavdahiya]

cluster-version-operator/pkg/cvo/sync.go

Lines 88 to 102 in fe673cb

	ObjectMeta: metav1.ObjectMeta{
	Name: fmt.Sprintf("operatorstatuses.%s", apis.OperatorStatusGroupName),
	Namespace: metav1.NamespaceDefault,
	},
	Spec: apiextv1beta1.CustomResourceDefinitionSpec{
	Group: apis.OperatorStatusGroupName,
	Version: "v1",
	Scope: "Namespaced",
	Names: apiextv1beta1.CustomResourceDefinitionNames{
	Plural: "operatorstatuses",
	Singular: "operatorstatus",
	Kind: "OperatorStatus",
	ListKind: "OperatorStatusList",
	},
	},

only exists to not break operators that are using the old CRD. that will be removed very soon.

[wking]

... only exists to not break operators that are using the old CRD...

So then should this PR be renaming newOperatorStatusBuilder -> newClusterOperatorBuilder and similar too? Or if you really want to use WithKind("ClusterOperator") to register newOperatorStatusBuilder, can you explain why the names should no longer match? And if all of this is terribly obvious to folks with more experience with CRDs (i.e. not me ;), feel free to ignore ;).

[abhinavdahiya]

So then should this PR be renaming newOperatorStatusBuilder -> newClusterOperatorBuilder and similar too? Or if you really want to use WithKind("ClusterOperator") to register newOperatorStatusBuilder, can you explain why the names should no longer match?

The builder name newOperatorStatusBuilder seemed still valid to me because it only operates on the status for clusteroperator which is operator's status.

[abhinavdahiya]

@wking #40 (comment) WDYT?

[wking]

The builder name newOperatorStatusBuilder seemed still valid to me because it only operates on the status for clusteroperator which is operator's status.

Sorry for the delay. I'm trying to wrap my head around operatorstatus.go, but here's where I'm at now:

• I think the log messages here and here also need OperatorStatus -> ClusterOperator now that we're getting the information from inside a ClusterOperator resource.
• I think readOperatorStatusV1OrDie should be renamed to readClusterOperatorV1OrDie because it's returning an *osv1.ClusterOperator.
• I think OperatorstatusV1Client is a strange name for a client whose only methods are RESTClient and ClusterOperators.
• I am ambivalent about newOperatorStatusBuilder.
• I'm fine with waitForOperatorStatusToBeDone.

Of the first three points, I'm fine with you picking them up here or punting them to follow-up work. Thoughts?

[abhinavdahiya]

@wking

I think the log messages here and here also need OperatorStatus -> ClusterOperator now that we're getting the information from inside a ClusterOperator resource.

Done

I think readOperatorStatusV1OrDie should be renamed to readClusterOperatorV1OrDie because it's returning an *osv1.ClusterOperator.

Done

I think OperatorstatusV1Client is a strange name for a client whose only methods are RESTClient and ClusterOperators.

That is generated code as the group for the api is operatorstatus.openshift.io, can't do anything...

I am ambivalent about newOperatorStatusBuilder.

Changed to newClusterOperatorBuilder

I'm fine with waitForOperatorStatusToBeDone.

Yeah, keeping it as is.

[wking]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [abhinavdahiya,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wking]

/retest

[abhinavdahiya]

/test e2e-aws