Print kuttl logs if kuttl tests cannot be started

[andreasgerstmayr]

Description of the change:
Print kuttl logs if kuttl tests cannot be started (for example when specifying an inaccessible path in crdDir).

Motivation for the change:
The empty kuttl test suite was found error message is not helpful in case kuttl cannot be started.

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

[everettraven]

@andreasgerstmayr Thanks for taking the time to make this contribution!

I am having a hard time trying to understand what this change is trying to accomplish. To me it seems like the motivation is to improve the error message of empty kuttl test suite was found, but my understanding of the changes made in this PR don't seem to reflect that.

All that being said, I'm by no means a SME in regards to scorecard and how it handles kuttl tests.

@theishshah Would you mind taking a look at this?

[andreasgerstmayr]

Hi @everettraven!

Sorry, I should have added more context to this PR's description.
I've created a minimal reproducible example to demonstrate the effects of this PR:
https://github.com/andreasgerstmayr/operator-sdk/tree/pr-6225-repro

This example includes a test with the current version (quay.io/operator-framework/scorecard-test-kuttl:v2.1) and a test with the code changes of this PR (docker.io/andreasgerstmayr/scorecard-test-kuttl:dev):

$ operator-sdk scorecard ./bundle
--------------------------------------------------------------------------------
Image:      docker.io/andreasgerstmayr/scorecard-test-kuttl:dev
Labels:
	"suite":"kuttlsuite"
	"test":"kuttltest1"
Results:
	State: unknown
	Log:
		2022/12/20 20:14:08 kutt-test config testdirs is overridden with args: [ /bundle/tests/scorecard/kuttl ]
		=== RUN   kuttl
		    harness.go:460: starting setup
		    harness.go:251: running tests using configured kubeconfig.
		    harness.go:511: cleaning up
		    harness.go:568: removing temp folder: ""
		    harness.go:593: fatal error installing crds: lstat ./tests/_build/crds/: no such file or directory
		--- FAIL: kuttl (0.05s)
		FAIL


--------------------------------------------------------------------------------
Image:      quay.io/operator-framework/scorecard-test-kuttl:v2.1
Labels:
	"suite":"kuttlsuite"
	"test":"kuttltest1"
Results:
	State: fail

	Errors:
		empty kuttl test suite was found

In the current version (quay.io/operator-framework/scorecard-test-kuttl:v2.1), the error message reads empty kuttl test suite was found, which shadows the actual error: kuttl couldn't start due to fatal error installing crds: lstat ./tests/_build/crds/: no such file or directory in this case.

afaics if kuttl can't start it won't emit any testsuites, and therefore the current logic prints empty kuttl test suite was found, which is not helpful in this case.

[andreasgerstmayr]

(for reference, open-telemetry/opentelemetry-operator#1252 led me to investigate this error message of the quay.io/operator-framework/scorecard-test-kuttl container)

[everettraven]

@andreasgerstmayr Thanks for providing more information! The motivation behind this makes much more sense to me now - I will take another look at the changes with this new perspective :)

[everettraven]

@andreasgerstmayr After re-evaluating the changes I have some suggestions.

If you disagree with any of the requested changes, let me know. I am happy to have as much discussion as necessary to make sure we are making the changes that make the most sense :)

[andreasgerstmayr]

When we put the kuttl logs in the Error field, we get the following output:

--------------------------------------------------------------------------------
Image:      docker.io/andreasgerstmayr/scorecard-test-kuttl:dev
Labels:
	"suite":"kuttlsuite"
	"test":"kuttltest1"
Results:
	State: fail

	Errors:
		no kuttl test suite was found. kuttl may not have run successfully | kuttl logs:
2022/12/21 11:51:54 kutt-test config testdirs is overridden with args: [ /bundle/tests/scorecard/kuttl ]
=== RUN   kuttl
    harness.go:460: starting setup
    harness.go:251: running tests using configured kubeconfig.
    harness.go:511: cleaning up
    harness.go:568: removing temp folder: ""
    harness.go:593: fatal error installing crds: lstat abc: no such file or directory
--- FAIL: kuttl (0.06s)
FAIL

afaics multi-line strings in the Log field get intended, but not in the Errors field.
I was thinking, wouldn't it be good if we always show the kuttl logs in an error case (you can never have enough logs when encountering an error state)?

I've updated the PR, and now we get the following output:

--------------------------------------------------------------------------------
Image:      docker.io/andreasgerstmayr/scorecard-test-kuttl:dev
Labels:
	"suite":"kuttlsuite"
	"test":"kuttltest1"
Results:
	State: fail

	Errors:
		no kuttl test suite was found. kuttl may not have run successfully
	Log:
		2022/12/21 12:50:40 kutt-test config testdirs is overridden with args: [ /bundle/tests/scorecard/kuttl ]
		=== RUN   kuttl
		    harness.go:460: starting setup
		    harness.go:251: running tests using configured kubeconfig.
		    harness.go:511: cleaning up
		    harness.go:568: removing temp folder: ""
		    harness.go:593: fatal error installing crds: lstat abc: no such file or directory
		--- FAIL: kuttl (0.06s)
		FAIL

What do you think?

[everettraven]

@andreasgerstmayr Thanks for making these changes and improving upon my original suggestion! These changes look great to me!

Only one final thing before approval - since this change introduces user facing changes to scorecard, would you mind adding a changelog (only marking my review as "Requesting Changes" because of this - once it is done I will approve 😄 )?

[andreasgerstmayr]

Done, thanks for the review!

[everettraven]

/lgtm

[laxmikantbpandhare]

/lgtm

[everettraven]

Closing and reopening to try and pick up the fixed links that are causing the sanity check to fail