kudobuilder / kudo

Kubernetes Universal Declarative Operator (KUDO)
https://kudo.dev
Apache License 2.0
1.18k stars 103 forks source link

Fixing false warnings for the `KudoOperatorTask` #1718

Closed zen-dog closed 3 years ago

zen-dog commented 3 years ago

Summary: in many cases, the task's OperatorVersion is being set by the dependency resolver during the package installation, so that it is valid for the field to be empty during verification.

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

ANeumann82 commented 3 years ago

LGTM, but I don't quite understand the summary. It's saying that this is being set by the dependency resolver so it's valid to be empty during verification, but we are adding an error here when it's empty? Or am I missing something? What does this have to do with false warnings if this is setting an error?

Yeah, took me a minute to understand it as well - Basically, the PR removes one of the checks that is one in Task.build, which is ok, because when the Operator is installed it does the lookup on installation.

So the in_cluster_resolver on the client (CLI) is ok with an unset OV, as it searches through the repository to find one. The in_cluster_resolver on the server (controller) is not ok with an unset OV, as it only tries to fetch an installed one.

I think I would prefer to have the in_cluster_resolver for the server to be adjusted, but this is ok for now...

zen-dog commented 3 years ago

I think I would prefer to have the in_cluster_resolver for the server to be adjusted, but this is ok for now...

The idea behind the server in_cluster_resolver is to only check the existing dependencies in case e.g. somebody created the OV/I manually and forgot the dependencies. So it is basically a sanity check:

maybe we should rename it to smth. like in_cluster_dependency_checker to avoid this confusion. wdyt @ANeumann82 ?