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

Example and docs for ability to copy to and from container for #1164 #1989

Conversation

sumitanvekar
Copy link
Contributor

Created example to demonstrate ability to copy to and from container using Generic Container.
Added example into advanced_options section.

Created for #1164

…sing Generic Container.

Added example into advanced_options section.
…were passing in my local machine but I figured, they could fail on CI pipelines
@rnorth
Copy link
Member

rnorth commented Oct 20, 2019

This is cool, thank you @sumitanvekar!

I think my main item of feedback would be to ask for this to appear in the features/files.md docs page - it should be its own section in the 'Files and volumes' page.

@sumitanvekar
Copy link
Contributor Author

@rnorth moved docs to files.md as per your comment. Thanks for your review!

@sumitanvekar
Copy link
Contributor Author

Couple tests fails for no apparent reason (from logs, they seemed to be flaky tests). So, I did an empty commit to trigger checks again.

@bsideup
Copy link
Member

bsideup commented Oct 20, 2019

@sumitanvekar please do not trigger full CI if you see some flaky tests, it creates a big load on our CI environments.
Instead, we will rerun the tests of that particular module when we see it during the review.

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

There are tests (in core/) for copy to/from already, no need to have a separate one in docs/examples. I think we should refer them from the docs and maybe cleanup a bit to make them look better in the docs.

@sumitanvekar
Copy link
Contributor Author

@bsideup I am not sure if we should use core test class for documentation since they will get polluted with commented code blocks which are required for documentation. However, if we are planning to get rid of all docs example code to avoid duplication, I will update pr with your recommendations.

On a separate note, do you want to add a section in contributing.md to mention about the flaky tests and process for that? That will help newbies. Happy to do a separate pr for it.

@rnorth
Copy link
Member

rnorth commented Jan 24, 2020

Looks like there are some missing imports in CopyFileToContainerTest

@rnorth
Copy link
Member

rnorth commented Jan 25, 2020

I've pushed fixed imports to the source branch.

@stale
Copy link

stale bot commented Apr 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale bot added the stale label Apr 25, 2020
@sumitanvekar
Copy link
Contributor Author

@rnorth @bsideup any chance this can get merged? Or does it need something?

@stale stale bot removed the stale label Apr 26, 2020
@rnorth
Copy link
Member

rnorth commented May 1, 2020

Let's merge it - sorry @sumitanvekar that this has taken so long.

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Has been updated per requests.

@rnorth rnorth merged commit 88849ac into testcontainers:master May 1, 2020
@rnorth
Copy link
Member

rnorth commented May 3, 2020

Ohoh, this has broken our build on Windows. I'll revert, and fix up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants