-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Example and docs for ability to copy to and from container for #1164 #1989
Conversation
…sing Generic Container. Added example into advanced_options section.
…were passing in my local machine but I figured, they could fail on CI pipelines
This is cool, thank you @sumitanvekar! I think my main item of feedback would be to ask for this to appear in the |
@rnorth moved docs to files.md as per your comment. Thanks for your review! |
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. |
@sumitanvekar please do not trigger full CI if you see some flaky tests, it creates a big load on our CI environments. |
There was a problem hiding this 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.
@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. |
Looks like there are some missing imports in |
I've pushed fixed imports to the source branch. |
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. |
Let's merge it - sorry @sumitanvekar that this has taken so long. |
There was a problem hiding this 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.
Ohoh, this has broken our build on Windows. I'll revert, and fix up. |
Created example to demonstrate ability to copy to and from container using Generic Container.
Added example into advanced_options section.
Created for #1164