[KEP-0009] feat: add expression based assertions #576
Conversation
|
|
||
| ResourceRefs []TestResourceRef `json:"resourceRefs,omitempty"` | ||
|
|
||
| AssertExpressions AnyAllExpressions `json:"assertExpressions,omitempty"` |
There was a problem hiding this comment.
This does not follow the structure in the KEP: assert(Any|All).celExpr. Is it simply the prototype of an old iteration of the KEP?
There was a problem hiding this comment.
In hindsight,
assert:
any:
...
all:
...would be less verbose. WDYT?
There was a problem hiding this comment.
I think the verbosity is the nearly same - the number of letters is the same and the KEP version has one colon fewer.
And the KEP version is flatter, less indentation to think about, so I'd be for sticking to the KEP version. 🤷🏻
| return gvk | ||
| } | ||
|
|
||
| func NewClient(kubeconfig, context string) func(bool) (client.Client, error) { |
There was a problem hiding this comment.
This file is already very long as is. Can we perhaps move this (maybe along with NewRetryClient) into the .../pkg/k8s package?
There was a problem hiding this comment.
Good point but that creates an import cycle. :(
package command-line-arguments
imports github.com/kudobuilder/kuttl/pkg/kuttlctl/cmd
imports github.com/kudobuilder/kuttl/pkg/k8s
imports github.com/kudobuilder/kuttl/pkg/test/utils
imports github.com/kudobuilder/kuttl/pkg/k8s: import cycle not allowedThere was a problem hiding this comment.
Maybe we could raise a separate PR to refactor the file?
There was a problem hiding this comment.
Good idea. Without looking at details, I think pkg/k8s should be the leaf? 🤔
| @@ -1321,3 +1398,29 @@ func Kubeconfig(cfg *rest.Config, w io.Writer) error { | |||
| }, | |||
| }, w) | |||
| } | |||
|
|
|||
| func constructGVK(apiVersion, kind string) schema.GroupVersionKind { | |||
There was a problem hiding this comment.
This is a nice helper method, but it would be even more powerful it you
- changed it from a bare
functo a method ofTestResourceRef - made it also internally create an
*Unstructured:gvk := constructGVK(resourceRef.ApiVersion, resourceRef.Kind) referencedResource := &unstructured.Unstructured{} referencedResource.SetGroupVersionKind(gvk) - and called it
NewUnstructuredor similar.
Thinking about it, it might make sense to have it also return a NamespacedName and an error in case crucial fields (like name and kind) are missing. On one hand, if we don't do any validation here, then the client would probably catch such garbage later. But on the other hand, we could use such validation function earlier, just after loading the assert files, in order to give user feedback on critical errors ASAP.
When you're at it, please also add a couple of test cases for it.
There was a problem hiding this comment.
NamespacedName/*unstructured.Unstructured would be required when we evaluate the expressions. If the goal is also to validate, we should have two separate methods:
func (t *TestResourceRef) BuildResourceReference() (namespacedName types.NamespacedName, referencedResource *unstructured.Unstructured)
func (t *TestResourceRef) Validate() (error)
pkg/test/utils/kubernetes.go
Outdated
| env, err := cel.NewEnv() | ||
| if err != nil { | ||
| return []error{fmt.Errorf("failed to create environment: %w", err)} | ||
| } | ||
|
|
||
| for k := range variables { | ||
| env, err = env.Extend(cel.Variable(k, cel.DynType)) | ||
| if err != nil { | ||
| return []error{fmt.Errorf("failed to add resource parameter '%v' to environment: %w", k, err)} | ||
| } | ||
| } | ||
|
|
||
| for _, expr := range expressions.Any { | ||
| ast, issues := env.Compile(expr) | ||
| if issues != nil && issues.Err() != nil { | ||
| return []error{fmt.Errorf("type-check error: %s", issues.Err())} | ||
| } | ||
|
|
||
| prg, err := env.Program(ast) | ||
| if err != nil { | ||
| return []error{fmt.Errorf("program constuction error: %w", err)} | ||
| } | ||
|
|
||
| out, _, err := prg.Eval(variables) | ||
| if err != nil { | ||
| return []error{fmt.Errorf("failed to evaluate program: %w", err)} | ||
| } | ||
|
|
||
| logger.Logf("expression '%v' evaluated to '%v'", expr, out.Value()) | ||
| if out.Value() != true { | ||
| errs = append(errs, fmt.Errorf("failed validation, expression '%v' evaluated to '%v'", expr, out.Value())) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Please make this a separate function that can be unit-tested without having to provide a k8s client.
| variables[resourceRef.Id] = referencedResource.Object | ||
| } | ||
|
|
||
| env, err := cel.NewEnv() |
There was a problem hiding this comment.
Calling cel.NewEnv() may not be enough, you probably want to enable a couple of options/libs.
| ast, issues := env.Compile(expr) | ||
| if issues != nil && issues.Err() != nil { | ||
| return []error{fmt.Errorf("type-check error: %s", issues.Err())} | ||
| } | ||
|
|
||
| prg, err := env.Program(ast) |
There was a problem hiding this comment.
It would be great to compile the program at load time, there's no point running the test if the compilation fails.
This PR adds CEL-expression based assertions to `TestAsserts`. See https://github.com/kudobuilder/kuttl/blob/main/keps/0009-expression-based-assertions.md for more details. Signed-off-by: Kumar Mallikarjuna <[email protected]>
Signed-off-by: Kumar Mallikarjuna <[email protected]>
Signed-off-by: Kumar Mallikarjuna <[email protected]>
kumar-mallikarjuna
force-pushed
the
KEP-0009
branch
from
3c7d6fe
to
0e43475
Compare
What this PR does / why we need it:
This PR adds CEL-expression based assertions to
TestAsserts. See https://github.com/kudobuilder/kuttl/blob/main/keps/0009-expression-based-assertions.md for more details.Fixes #562