refactor: better resource cleanup handling #419
Conversation
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
eddycharly
requested review from
kensipe,
asekretenko and
kaiwalyajoshi
as code owners
pkg/test/case.go
Outdated
| @@ -355,7 +344,7 @@ func (t *Case) Run(test *testing.T, tc *report.Testcase) { | |||
| tc.Assertions += len(testStep.Asserts) | |||
| tc.Assertions += len(testStep.Errors) | |||
|
|
|||
| if errs := runStep(test, t, testStep, ns); len(errs) > 0 { | |||
| if errs := testStep.Run(test, ns.Name, t.SkipDelete); len(errs) > 0 { | |||
There was a problem hiding this comment.
Could it make sense to add a new SkipDelete field to the Step struct, to avoid passing this around as a func param?
There was a problem hiding this comment.
Not sure it would make things better, currently this is not overridable in a Step, this makes sense to me that it is the responsibility of the caller to pass the flag.
I don't have a strong opinion though, if you prefer the field based approach I will use that.
WDYT ?
There was a problem hiding this comment.
I don't have a strong opinion on this. So far the func has few params. What I don't like is funcs with a lot of params - especially of the same type. :-)
There was a problem hiding this comment.
Switched to a struct field.
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
eddycharly
requested review from
erikgb
and removed request for
kensipe,
kaiwalyajoshi and
asekretenko
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
There was a problem hiding this comment.
integration tests need updating for completion
./bin/golangci-lint --timeout 5m run --build-tags integration
pkg/test/step_integration_test.go:443:28: not enough arguments in call to step.Run
have (string)
want (*"testing".T, string) (typecheck)
errs := step.Run(namespace)
^
pkg/test/step_integration_test.go:461:33: not enough arguments in call to step.Run
have (string)
want (*"testing".T, string) (typecheck)
errors := step.Run("irrelevant")
^
pkg/test/step_integration_test.go:479:33: not enough arguments in call to step.Run
have (string)
want (*"testing".T, string) (typecheck)
errors := step.Run("irrelevant")
|
for awareness... I commonly run the following as a starting point of review:
|
|
I will fix this tomorrow. How about running this in a gh workflow ? |
|
yeah... when I noticed the passing CI... I had the same thought. I will look into that. |
|
well... now I remember some issues we had in the past but haven't been resolved. For some reason CircleCI wouldn't run PRs from non-committers. So the full CI isn't run (just the DCO). I worked with circle to help resolve that but didn't get it resolved. We were looking to move to Github Actions in part to solve that... but it also reduces the number of CI dependencies. Probably need to refocus on that. |
|
that's what #337 was intended to solve. |
Signed-off-by: Ken Sipe <kensipe@gmail.com>
There was a problem hiding this comment.
thanks @eddycharly . I pushed the integration test refactors. I'll merge after good CI runs. Thanks for the contribution!
What this PR does / why we need it:
This PR improves resource cleanup handling.