fix: modify olm-status-descriptor-test in scorecard

[varshaprasad96]

Description of the change:
Modify olm-status-descriptor-test to not check the presence of
status field in CR, instead validate only if status-descriptors
are present in owned CRDs.

cc: @jmccormick2001

Motivation for the change:
Fixes: #3999

Checklist

If the pull request includes user-facing changes, extra documentation is required:

• Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
• Add or update relevant sections of the docs website in website/content/en/docs

[estroz]

Needs to be cherry-picked to v1.0.x

[camilamacedo86]

Just a few nits to address. Otherwise, it shows fine for me. 👍

[varshaprasad96]

/cherry-pick v1.0.x

[openshift-cherrypick-robot]

@varshaprasad96: new pull request created: #4088

In response to this:

/cherry-pick v1.0.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[estroz]

I thought the whole point of this PR was to remove this check entirely @varshaprasad96?

[varshaprasad96]

Since having Status field is suggested according the k8s-api conventions, have modified this check to add suggestions. Also this unit test ensures that if Object descriptor is nil, the scorecard test still passes.

[varshaprasad96]

Instead of failing when status is not present in CR, suggestions are added. And we directly proceed with checking if the statusDescriptor is present in CRD or not. If the users input as described in #3999 is given with no status field in CR, this test would pass.

[estroz]

A CR from alm-examples should never have a status according to #3999 (comment), so we shouldn't be suggesting that one be added. Whether the CRD defines a status field for a resource in its validation is another story.

[varshaprasad96]

Hmm..okay. Because based on the comment, I tried creating a resource through console UI - with and without status field. And it worked in both the cases. And to verify if such CSVs exist with status, just randomly went through the example community operators and found this.

But considering we remove that validation check for that, can we remove the suggestion, or do you suggest we remove the entire [StatusDescriptor](

operator-sdk/internal/scorecard/tests/olm.go

Lines 232 to 284 in 9d27e22

	func checkOwnedCSVDescriptors(cr unstructured.Unstructured, csv *operatorsv1alpha1.ClusterServiceVersion,
	descriptor string, r scapiv1alpha3.TestResult) scapiv1alpha3.TestResult {
	if cr.Object[descriptor] == nil {
	r.State = scapiv1alpha3.FailState
	return r
	}
	block := cr.Object[descriptor].(map[string]interface{})
	var crd *operatorsv1alpha1.CRDDescription
	for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
	if owned.Kind == cr.GetKind() {
	crd = &owned
	break
	}
	}
	if crd == nil {
	msg := fmt.Sprintf("Failed to find an owned CRD for CR %s with GVK %s", cr.GetName(), cr.GroupVersionKind().String())
	r.Errors = append(r.Errors, msg)
	r.State = scapiv1alpha3.FailState
	return r
	}
	if descriptor == statusDescriptor {
	for key := range block {
	for _, statDesc := range crd.StatusDescriptors {
	if statDesc.Path == key {
	delete(block, key)
	break
	}
	}
	}
	}
	if descriptor == specDescriptor {
	for key := range block {
	for _, specDesc := range crd.SpecDescriptors {
	if specDesc.Path == key {
	delete(block, key)
	break
	}
	}
	}
	}
	for key := range block {
	r.Errors = append(r.Errors, fmt.Sprintf("%s does not have a %s descriptor", key, descriptor))
	r.Suggestions = append(r.Suggestions, fmt.Sprintf("Add a %s descriptor for %s", descriptor, key))
	r.State = scapiv1alpha3.FailState
	}
	return r
	}

) validation test?

Shouldn't we still check if statusDescriptor is present in CRD?

[estroz]

I shouldn't have said "never" above. The alm-examples can have status blocks, but they should be treated as not existing. Therefore I'm only suggesting the cr.Object[statusDescriptor] == nil condition be removed. Everything else is fine.

[varshaprasad96]

Got it. Will create a follow up PR for this.