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

Return a Reader from the Exec function #435

Merged
merged 1 commit into from
May 5, 2022

Conversation

johanoskarsson
Copy link

@johanoskarsson johanoskarsson commented Apr 28, 2022

This should allow the user to read the logs of the command execution. I've tested this quickly locally and it seems to work as expected.
Pushing this so I can get some CI feedback.
I believe this is similar to what was asked for in #126

logconsumer_test.go Outdated Show resolved Hide resolved
@johanoskarsson
Copy link
Author

Thanks I had them the wrong way around first!

@mdelapenya
Copy link
Member

Hey @johanoskarsson, I took a look at your PR and it seems it’s outdated (created before the commit that fixed the netlify deployment in main):

This branch is 2 commits ahead, 13 commits behind testcontainers/testcontainers-go:main

Could you rebase and push? Thank you in advance for your work here

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM! Let's wait for the CI

Thanks

@codecov
Copy link

codecov bot commented Apr 30, 2022

Codecov Report

Merging #435 (ca89845) into main (cd84bb3) will increase coverage by 0.14%.
The diff coverage is 61.53%.

@@            Coverage Diff             @@
##             main     #435      +/-   ##
==========================================
+ Coverage   65.13%   65.28%   +0.14%     
==========================================
  Files          19       19              
  Lines        1179     1184       +5     
==========================================
+ Hits          768      773       +5     
  Misses        304      304              
  Partials      107      107              
Impacted Files Coverage Δ
container.go 87.23% <ø> (ø)
wait/wait.go 100.00% <ø> (ø)
docker.go 65.57% <54.54%> (+0.31%) ⬆️
wait/exec.go 92.00% <100.00%> (ø)
wait/host_port.go 56.14% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd84bb3...ca89845. Read the comment docs.

@mdelapenya mdelapenya added the breaking change Causing compatibility issues. label Apr 30, 2022
@mdelapenya mdelapenya self-assigned this Apr 30, 2022
@mdelapenya
Copy link
Member

NIT: before merging this, could you add a test (or an assertion in the existing tests) where we are consuming the Reader? I'm checking that we are always omitting it in the current tests, and would like to see it covered.

@mdelapenya
Copy link
Member

BTW, I'm tagging this PR as breaking-change because we are changing the signature of an exposed method. Because we are not in 1.0.0, it's fine to break it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Causing compatibility issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants