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

Added CopyFileFromContainer to DockerContainer #347

Merged
merged 3 commits into from
Sep 13, 2021

Conversation

codepitbull
Copy link
Contributor

Why ?

While working on a k3s-container I needed to extract the kubeconfig from the running container and discovered that there is no CopyFileFromContainer available.

What ?

Added CopyFileFromContainer to DockerContainer.
Added Unit-test for the new Function.

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.

Thanks for this contribution @codepitbull, this LGTM!

@mdelapenya
Copy link
Member

As a follow-up, would you see it doable to contribute an implementation to the compose provider? 🤔

@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #347 (28d20ca) into master (6185475) will increase coverage by 0.58%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   61.53%   62.11%   +0.58%     
==========================================
  Files          15       15              
  Lines         993     1011      +18     
==========================================
+ Hits          611      628      +17     
+ Misses        290      286       -4     
- Partials       92       97       +5     
Impacted Files Coverage Δ
container.go 85.29% <ø> (ø)
docker.go 65.13% <61.53%> (+1.14%) ⬆️

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 6185475...28d20ca. Read the comment docs.

@codepitbull
Copy link
Contributor Author

I will checxk to add it to the ComposeProvider

@codepitbull
Copy link
Contributor Author

About the Compose Part: There are no containers exposed.
The compose implementation as it is really just runs docker compose and can check for availability of a container.
It doesn't even allow copying files into a running container.
from what I see this qould take quite a big effort to achieve.

@mdelapenya
Copy link
Member

About the Compose Part: There are no containers exposed.
The compose implementation as it is really just runs docker compose and can check for availability of a container.
It doesn't even allow copying files into a running container.
from what I see this qould take quite a big effort to achieve.

Sure thing. I can create an issue to add the support to the compose provider. Meanwhile, this LGTM

@mdelapenya mdelapenya requested a review from gianarb August 19, 2021 19:58
@gianarb
Copy link
Member

gianarb commented Aug 24, 2021

Hello! Thank you for your contribution! I had a chat with @mdelapenya via Slack and I would like to know what you think about this:

	CopyFileToContainer(ctx context.Context, hostFilePath string, containerFilePath string, fileMode int64) error
	CopyFileFromContainer(ctx context.Context, filePath string) ([]byte, error)

I see how your function will may come from what we have done with CopyFileToContainer but now that I see it if I think we made a mistake there (I think we should deprecate that in favor of something else).

I think the right signature should be:

	CopyFileFromContainer(ctx context.Context, filePath string) (io.ReadCloser, error)

This works a bit better over the network and it give more control to the end user who can decide what to do with an io.Reader.

I understand the extra code you wrote sounds like a useful utility function but at the end it can be replaced with a few line as explained here https://pkg.go.dev/io/ioutil#ReadAll and I don't think it should be something to include as part of the testcontainer library.

What do you think?

@codepitbull
Copy link
Contributor Author

Hey @gianarb,
thanks for your feedback :)
I have updated my Pr to use the signature you suggested.
With your suggestions in mind I also thought it would be better to eliminate all eager reading.
I wrapped the whole tar reader to enable closing of the underlying ReaderCloser and to return a valid object.
You think this would work better?

Cheers,
Jochen

@codepitbull
Copy link
Contributor Author

Hey @mdelapenya or @gianarb ,
are you interested in this? Anything else you would like to see changed/added in here?
Cheers,
Jochen

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.

This LGTM! Thanks again for this contribution and for your patience. I usually wait for @gianarb performing the merges, so let's wait for that review.

@mdelapenya
Copy link
Member

mdelapenya commented Sep 9, 2021

BTW, the GH action complains about source formatting in the docker.go file: https://github.com/testcontainers/testcontainers-go/pull/347/checks?check_run_id=3516875570

Could you run ./scripts/checks.sh and verify the issue? 🙏

@codepitbull
Copy link
Contributor Author

@mdelapenya fixed it, forgot to run "go fmt" ...

@codepitbull
Copy link
Contributor Author

And just so you know what this is for:
Thanks to a few hints from @bsideup I was able to implement k3s-based integrationtests for k8s.
The only missing piece was this function.

Here is the example of what I am using it for:
https://github.com/codepitbull/k8sit/blob/main/gok3s/k8sit/testcontainer_test.go

@bsideup
Copy link
Member

bsideup commented Sep 11, 2021

@codepitbull omg this is great! 💯 FYI the original k3s minimal container was created by @rnorth, I am just a messenger here :D

@gianarb gianarb merged commit 19e857f into testcontainers:master Sep 13, 2021
@gianarb
Copy link
Member

gianarb commented Sep 13, 2021

Thank you for this!!

@codepitbull
Copy link
Contributor Author

Thanks to you and the other maintainers!
We are really happy with it for our integration tests :)

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.

4 participants