commands/.../scorecard: add multi-cr support #1236
Conversation
AlexNPavel
added
kind/feature
AlexNPavel
requested review from
estroz,
hasbro17,
joelanford,
lilic,
shawn-hurley and
theishshah
openshift-ci-robot
added
the
size/XL
|
There's extra work to be done for OLM-deployed operators with multiple CR's, but I'll do that in a follow-up PR. |
|
This is ready for review. |
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
Did a quick review and looks pretty good! I still need to spend more time trying it out.
...oy/olm-catalog/memcached-operator/0.0.3/memcached-operator.v0.0.3.clusterserviceversion.yaml
Show resolved
Hide resolved
test/test-framework/pkg/apis/cache/v1alpha1/zz_generated.deepcopy.go
Outdated
Show resolved
Hide resolved
test/test-framework/pkg/controller/memcachedrs/memcachedrs_controller.go
Show resolved
Hide resolved
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
Just some minor pointer-related suggestions and a question about how one of the scenarios with multiple CRs in the CSV resources test.
|
New changes are detected. LGTM label has been removed. |
| } | ||
|
|
||
| // Run - implements Test interface | ||
| func (t *CRDsHaveResourcesTest) Run(ctx context.Context) *TestResult { |
There was a problem hiding this comment.
I missed this pointer return yesterday. Can we return a TestResult here? If so, I guess this would apply to all of the tests.
There was a problem hiding this comment.
Yeah, we can change that to return the struct instead of a struct pointer. It's outside of the scope of changes in this PR though, so I'll do that in a separate PR.
4 participants
Description of the change: Add basic multi-CR support for operator-sdk scorecard. This also adds a new CR to the
test/test-framework, that is the has identical functionality as the original but uses replica sets, for testing (which is why this PR is so large; much of it is just adding another CR to memcached-operator).Motivation for the change: See issue #984.
While this PR will add some early, basic support for multiple CRs in the scorecard, there are some major difficulties for many operators that implement multiple CRs. The scorecard assumes that the CR being created will result in the operator responding and creating/modifying resources. However, in many operators with multiple CRDs, the additional CRDs are more of a helper resource rather than the full definition of an application. This can be seen in the metering operator for example. The metering operator has 7 CRDs in total, and many of them are not intended to be run individually. The PrestoTable CRD specifically mentions that it's used "Under the hood" in its description.
This leads to some big problems as we cannot treat multi-CR operators the same as we do single-CR operators. How to handle multi-CR operators beyond what this PR does (just running the same tests on each CR independently) will require more discussion.