Add support for fetching container logs from a given time

[NuShoSinkuPomogliTebeTvoiSankcii]

Added ability to pass options to dockerode logs API for StartedContainer.
I need it to pass since parameter for example.

[netlify]

✅ Deploy Preview for testcontainers-node ready!

Name	Link
🔨 Latest commit	dcd4513
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-node/deploys/655f4d880eafcb0009f69856
😎 Deploy Preview	https://deploy-preview-670--testcontainers-node.netlify.app/features/containers
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[NuShoSinkuPomogliTebeTvoiSankcii]

Hello! @javierlopezdeancos

Can u please review my PR?

[NuShoSinkuPomogliTebeTvoiSankcii]

Hello! @kiview

Can u please review my PR? It's very simple

[javierlopezdeancos]

Hey @NuShoSinkuPomogliTebeTvoiSankcii can you add some description or link the issue to get context and leave it track?

[NuShoSinkuPomogliTebeTvoiSankcii]

@javierlopezdeancos
My changes added ability to pass options to dockerode logs API for StartedContainer.
I need it to pass since parameter for example.

[javierlopezdeancos]

Thanks to your clarification. is LGTM
I notice that I dont have permissions with my approval, probably @mdelapenya can take another look and approve to merge?

[mdelapenya]

@javierlopezdeancos My changes added ability to pass options to dockerode logs API for StartedContainer. I need it to pass since parameter for example.

Thanks for submitting this PR @NuShoSinkuPomogliTebeTvoiSankcii , could you please update the PR description with the motivation for it (the same in your comment). We need discoverability for the future us when going back to the code, and for eventual readers to check why some code was introduced in the project. It's on us to update the PR template including certain sections, like "What does this PR do?" and "Why is it important?" (PTAL to other testcontainers projects)

In the mean time, this PR looks good to me. Will merge it once the description is updated and the CI passes.

Thanks!

[mdelapenya]

Oh I forgot! Could you please add a test demonstrating the new behaviour? 🙏

[NuShoSinkuPomogliTebeTvoiSankcii]

@mdelapenya thanks for your advice! Added tests and found out that I missed some logic (added it to docker-container-client).

Also added description to my PR.

[NuShoSinkuPomogliTebeTvoiSankcii]

@mdelapenya Can you also publish new library version to npm after my PR will be merged, please?

[mdelapenya]

I added a few comments regarding the new opts variable.

Thanks!

[mdelapenya]

@mdelapenya Can you also publish new library version to npm after my PR will be merged, please?

Hey @NuShoSinkuPomogliTebeTvoiSankcii I'm the maintainer of testcontainers-go, helping out with the maintenance of the NodeJS flavour while @cristianrgreco is on PTO. I can help with the big picture of the library, where @javierlopezdeancos can support me with the Typescript specifics.

Said that, I'm afraid that we are going to slow down a bit the release cadence until we get more familiar with the codebase. But hopefully soon enough to see your changes released, for sure. So please be patient with us 🙏

[NuShoSinkuPomogliTebeTvoiSankcii]

@javierlopezdeancos I reverted my changes with overriding options. Just passing options for since parameter – actually it's the only thing that I need for now.

Also added test for this behaviour. Review it please. 🙏

[NuShoSinkuPomogliTebeTvoiSankcii]

@javierlopezdeancos I'm sorry for disturbing, but I will also be on leave (as @cristianrgreco 😄) after Friday for 2 weeks, so it will be nice to merge my PR before that date.

[cristianrgreco]

Hi @NuShoSinkuPomogliTebeTvoiSankcii, thanks for raising the PR. I'd prefer not to expose the underlying Docker API (dockerode) in Testcontainers' public API. This way we do not proxy breaking changes from dockerode/Docker when they happen. I see from the test you want to add support for fetching logs since a certain time, so let's implement support just for that and update the docs to explain how to use it.

If it is low level access to the Docker client you need, this PR will give you what you're after: #644

[NuShoSinkuPomogliTebeTvoiSankcii]

@cristianrgreco Hi! Done refactoring and docs.

About low level access – I understand the problem with breaking changes.
But for me it would be nice to have both low level dockerode API and high level tc-node at the same time through the same JS object. In your example it's produce more boilerplate with storing many different instances of the same object.

For example this PR. In my project I had to copy-paste this logic with dockerode interactions just to pass since parameter directly to dockerode container logs method.
If someone will need to pass tail/until args (from ContainerLogsOptions) to StartedContainer they will also need to extend options interface that I just created. Or create another container instance through container-runtime-client just for access logs with some extra options. It's not so convenient.

P.S. Glad to have you back!

[cristianrgreco]

Thanks for the response @NuShoSinkuPomogliTebeTvoiSankcii.

I think I understand your points, but I think this result is actually ideal: You identified a use case for fetching logs since a given time, you've raised a well documented PR with this behaviour, where a conflict when upgrading Dockerode results in a compile time error, which ensures we don't accidentally introduce breaking changes. Thanks to a level of indirection we can handle breaking changes in Dockerode without requiring changes from our users, IMO this is a win.

If you think supporting tail/until is a common use case then please feel free to raise more PRs, but to date this has not been asked for. If raising a PR is an inconvenience then adding a couple of lines of code to get the underlying Docker client I think is a compromise worthwhile for not having breaking changes.

The PR LGTM, than you for making the changes. I'll get this deployed shortly.

P.S. Glad to have you back!

Thanks 🙂