Skip to content

Container stop timeouts should be in milliseconds #962

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Container stop timeouts should be in milliseconds #962

wants to merge 6 commits into from

Conversation

digital88
Copy link
Contributor

#960

  • Added class Ms which provides conversion from ms to nanos and seconds.
  • Explicitly call this class' conversion methods when calling Docker APIs. Example:
...
log.debug(`Stopping container...`, { containerId: container.id });
const t = new Ms(opts?.timeout ?? 0).seconds();
await container.stop({ t });
...
  • Renamed parameters in all public APIs to tell that value in ms expected to be passed.
  • Renamed some internal fields and methods to tell that value in ms expected there.
  • Added docs for timeouts.
  • Updated old docs where timeouts are used.

@netlify Netlify
Copy link

netlify bot commented Mar 26, 2025 •
edited
Loading

✅ Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit ad8cd4a
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-node/deploys/68309b476c8fea00085bff52
😎 Deploy Preview https://deploy-preview-962--testcontainers-node.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@cristianrgreco cristianrgreco added enhancement New feature or request major An incompatible API change labels Mar 26, 2025
@cristianrgreco cristianrgreco added this to the 11.0.0 milestone Mar 26, 2025
cristianrgreco
cristianrgreco reviewed May 23, 2025
View reviewed changes
packages/testcontainers/src/container-runtime/clients/container/docker-container-client.ts
@@ -131,7 +130,7 @@ export class DockerContainerClient implements ContainerClient {
async stop(container: Container, opts?: { timeout: number }): Promise<void> {
try {
log.debug(`Stopping container...`, { containerId: container.id });
await container.stop({ t: opts?.timeout });
await container.stop({ t: toSeconds(opts?.timeout ?? 0) });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: these are the reasons why this is a breaking change

@cristianrgreco cristianrgreco changed the title Refactor of timeouts Container stop timeouts should be in milliseconds May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristianrgreco cristianrgreco cristianrgreco left review comments

Assignees
No one assigned
Labels
enhancement New feature or request major An incompatible API change
Projects
None yet
Milestone
11.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@digital88 @cristianrgreco