Skip to content
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.

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

Add dump command to allow container logs dumping #444

Closed
wants to merge 6 commits into from

Conversation

alexpusch
Copy link

A common use case in integration tests development and debugging is inspecting created container logs.
Currently the way to do so is to run the tests using the "keep" command and inspect the logs afterwards. This isn't a good fit for CI testing environment, and might not be ideal for local development as well.

This PR adds a new "dump" command that removes the containers just like "remove" but will dump container logs into a "./testcontainers" directory.

The logs filename is {container_name/image_name}_{stdout/stderr}_{current date in ISO format}.log

usage example: TESTCONTAINERS=dump cargo test

Copy link
Collaborator

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks! A few comments.


fn get_log_dump_dir_path() -> PathBuf {
env::current_dir()
.unwrap_or(PathBuf::from("/tmp"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is gonna fail on Windows. I'd say if we can't get the current directory, print a warning and lets not dump them.

Copy link
Author

Choose a reason for hiding this comment

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

On second thought, it might be better to allow passing the required path from outside - maybe an additional env var, or some syntax on the command env var i.e TESTCONTAINERS=dump=/tmp/target ? what do you think?

testcontainers/src/core/logs.rs Outdated Show resolved Hide resolved
testcontainers/src/core/container.rs Show resolved Hide resolved
@alexpusch
Copy link
Author

@thomaseizinger glad you like the feature :)
I added support for this in ContainerAsync as well.

In addition I changed to logs directory structure so that all logs from same test run will be saved in its own directory: ./testcontainers/{iso date}/{container name|image name}_{container_id}_{stdout|stderr}.log. I think this would provide a better user experience.
This is done by saving the date of the first log dump in a lazy static variable.

@alexpusch
Copy link
Author

@thomaseizinger what do you say? Any other comments to advance this PR?

@DDtKey
Copy link
Collaborator

DDtKey commented Apr 7, 2024

The ability to access logs is definitely helpful

However I'd consider more aligned interface with implementations in other languages, like java and golang

Also keep in mind, that we gonna deprecate CLI client soon (#563). So we actually can introduce this for http client only

Subjectively, I'd avoid configuration though env variable and dumping logs to filesystem. In addition there is an alternative: it could be achieved by not-removing container and accessing logs with docker cli.

@DDtKey
Copy link
Collaborator

DDtKey commented Apr 7, 2024

Let us know if you're still interested in implementing / actualizing this PR.
We actively revamp the project

@DDtKey DDtKey closed this Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants