feat(libraries): use chainsaw and crossplane as library #39
Conversation
Signed-off-by: Christopher Haar <[email protected]>
| ticker := time.NewTicker(t.options.LogCollectionInterval) | ||
| done := make(chan bool) | ||
| var mutex sync.Mutex |
There was a problem hiding this comment.
Looks like this mutex is used only inside logCollector, so it could be declared created inside logCollector instead of passing it in.
There was a problem hiding this comment.
There is a significant increase in dependencies, including chainsaw, crossplane as a library, and other indirect dependencies. Consider whether all these dependencies are necessary for this PR to prevent long-term maintenance issues. If some dependencies are only used in a specific module, can we limit their scope?
There was a problem hiding this comment.
think most of the dependencies are from chainsaw regarding auth plugins
| configuration.Spec.Discovery.TestFile = tf | ||
| configuration.Spec.Execution.Parallel = ptr.To(1) | ||
| configuration.Spec.Cleanup.SkipDelete = true |
There was a problem hiding this comment.
Do we need to set any other configuration field for preserving the old behavior?
There was a problem hiding this comment.
before we used the binary input parameters
${CHAINSAW}" test --test-dir %s --test-file %s --skip-delete --parallel 1
--test-dir %s
which maps to tests, err := discovery.DiscoverTests(tf, nil, false, t.options.Directory)
--test-file %s
which maps to configuration.Spec.Discovery.TestFile = tf
--skip-delete
which maps to configuration.Spec.Cleanup.SkipDelete = true
--parallel 1
which maps to configuration.Spec.Execution.Parallel = ptr.To(1)
chainsaw is using default when you not explicit set some of the inputs configuration, err := kconfig.DefaultConfiguration()
| log.Printf("- Using test file: %s\n", configuration.Spec.Discovery.TestFile) | ||
| log.Printf("- ApplyTimeout %v\n", configuration.Spec.Timeouts.Apply.Duration) | ||
| log.Printf("- AssertTimeout %v\n", configuration.Spec.Timeouts.Assert.Duration) | ||
| log.Printf("- CleanupTimeout %v\n", configuration.Spec.Timeouts.Cleanup.Duration) | ||
| log.Printf("- DeleteTimeout %v\n", configuration.Spec.Timeouts.Delete.Duration) | ||
| log.Printf("- ErrorTimeout %v\n", configuration.Spec.Timeouts.Error.Duration) | ||
| log.Printf("- ExecTimeout %v\n", configuration.Spec.Timeouts.Exec.Duration) | ||
| log.Printf("- Parallel %d\n", *configuration.Spec.Execution.Parallel) |
There was a problem hiding this comment.
We have some timeout override mechanisms in the template and example manifest files. I am curious about, can we have still this capability after the changes? Did we check this?
There was a problem hiding this comment.
we have 2 layers of configuration - default configuration and test specific overrides - we let chainsaw library handling the overrides like before
| defer cancel() | ||
|
|
||
| if err := runnerflags.SetupFlags(configuration.Spec); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
It used to be that the logs of the CLI were printed here in a stream and as they came in, is this the case now? Or are they printed once for a test execution and then all together?
There was a problem hiding this comment.
does this help to see the log flow in a uptest run?
| mutex.Lock() | ||
| log.Println(sc.Text()) | ||
| mutex.Unlock() | ||
| ctx, cancel := context.WithTimeout(context.Background(), timeout) |
There was a problem hiding this comment.
Just a comment about the timeouts. Since the chainsaw doesn't have global timeouts for test suites we created the context.WithTimeout and passed it to the exec.CommandContext. The aim is killing the all child processes also with this. Do you think are we able to protect this behavior?
Description of your changes
Fixes #
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested