feat(termination)!: make container termination timeout configurable #2926
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mdelapenya
added
feature
moogacs
changed the title
cleanup.go
Outdated
| @@ -16,21 +15,21 @@ type terminateOptions struct { | |||
| } | |||
|
|
|||
| // TerminateOption is a type that represents an option for terminating a container. | |||
| type TerminateOption func(*terminateOptions) | |||
| type TerminateOption func(*DockerContainer) | |||
There was a problem hiding this comment.
bug: This ties the container implementation to the concrete DockerContainer type, which would prevent other types implementing Container for example the incoming ollama change.
We could maintain the dedicated type but export it and the fields, which could then be embedded in others.
The challenge with that is you can't rely on the interface to ensure an implementation is complete so it may be better to bite the bullet and make them concrete options @mdelapenya thoughts?
There was a problem hiding this comment.
yeah, i had similar feeling, but wasn't sure if shall i introduce a separate type, when i was opening this PR i tried to make as minimal breaking change but seems it has to be. open for suggestions
i like the idea of maintain the dedicated type but export it and the fields
There was a problem hiding this comment.
I initially thought the dedicated type would work, but that wont ensure that all Container implementations support the options, which I'm not a fan of as a consumer would be left wondering why their options didn't work.
There was a problem hiding this comment.
the other thought i had before i come to this was introducing it in the ContainerRequest and add getter in the container interface
There was a problem hiding this comment.
I thought about setters too, but I think that would make it much harder to discover for users.
I can't think of any perfect solution ATM, something mull over.
There was a problem hiding this comment.
i have made a change and revert it to make sure it uses terminateOptions instead of the dependency on DockerContainer type.
also added more test to show case the usage and the changes of configuration
defaults of volumes is nil, timeout is 10 and context is passed already on Terminate() , lmk wdyt ?
…iner and introduce configuration test
There was a problem hiding this comment.
Thanks for fleshing out what this looks like, I've done a review on how it stands and will discuss with @mdelapenya in the new year the pros and cons of functional options vs require args.
| absPath, err := filepath.Abs(filepath.Join(".", "testdata", "hello.sh")) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Just so you're aware (no need to change) we've been using embed for this type recently to its easier to reuse.
//go:embed testdata/hello.sh
var helloSh []byteThen use:
ContainerFile{
Reader: bytes.NewReader(helloSh),
ContainerFilePath: "/hello.sh",
FileMode: 0o755,
}
|
@stevenh thanks for feedback, i have address the comments. looking forward, Happy new year 😊 |
What does this PR do?
Terminate()method with optional configuration for timeout configuration.terminationOptionsunder container to give the ability to make timeout, etc configurable via a functional option.Terminate()function.Why is it important?
Terminate()became broken with latest release version with latest release0.34.0Terminate()require handle for errors"is already in progress"testing.TBso with this change it will give the ability to configure the timeout and move the cleanup logic under termination
Related issues
a unit test added and shall be tested with all the existed pipelines