-
Notifications
You must be signed in to change notification settings - Fork 329
feat: Transferable #852
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
base: main
Are you sure you want to change the base?
feat: Transferable #852
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #852 +/- ##
=======================================
Coverage ? 79.68%
=======================================
Files ? 15
Lines ? 1216
Branches ? 193
=======================================
Hits ? 969
Misses ? 204
Partials ? 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8ddf5c6
to
8c29c86
Compare
Hi @rhoban13, what is the current status of this feature? What could I do to help? For what it's worth, I think the suggested interface is clear. I haven't tested it with my use cases yet though. |
Tagging a few maintainers seeking feedback @terry-docker @alexanderankin @Tranquility2 |
i think my current priorities are docs, anything that makes this library behave like the others (this PR is illustrating current state not improving it), so far the only new issue from the new release is someone complaining about warning logs - so i think that means that the new release was successful (either that or there are no more users of this library). anecdotally someone reached out on slack asking for a new release but they are only allowed to use it at work a week after it has been released. so there may be new issues in some time. |
wait, this adds Transferable - this is great - if we can also fix the immediate starting of the container this almost entirely fixes copying. just have to maybe see about creating folders automatically and i think we may have parity on copying files with the other impls |
Yes I was just about to update the PR title - my original intent was just some tests, but I found it easier to write those tests using TDD & just implement it at the same time. I think this brings the library a bit closer to feature parity with some others. I'll take a look at adding some tests around folders if it sounds like you're in agreement with the interface at a high level. |
Thanks! I'll try to run some tests then and will let you know how they went. |
This is independent of this PR, right? I presume you're referring to the fact that we're calling docker run instead of create+start? |
yeah this whole API is not going to behave as intended until we start doing create then start |
@@ -298,6 +308,50 @@ def _configure(self) -> None: | |||
# placeholder if subclasses want to define this and use the default start method | |||
pass | |||
|
|||
def with_copy_into_container( | |||
self, file_content: Union[bytes, pathlib.Path], destination_in_container: str, mode: int = 0o644 |
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.
Maybe the name file_content
here isn't great? Maybe source
or copy_source
?
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.
In java they all just take transferable and destination I think
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.
I refined the initial definition of Transferable
to now simply the be the union of those types. I kinda like that the signatures all just contain transferable: Transferable
. It was an easy update except for the case of passing args to the initializer, which I changed to more closely mirror the type passed to volumes
.
I ran some tests with this PR, it went well 🙂. I used this function to transfer directories: def transferables_from_directory(
source_dir: Path, dest_dir: Path, mode: int
) -> list[TransferSpec]:
transferables: list[TransferSpec] = []
for dirpath, _, filenames in source_dir.walk():
for name in filenames:
origin = dirpath / name
destination = dest_dir / dirpath.relative_to(source_dir) / name
transferables.append((origin, str(destination), mode))
return transferables |
I updated the implementation so all these methods should work if the transferable Path is a directory as well. |
In #676 for copying to/from
DockerContainer
, it was suggested that we should clarify the interface & usage a bit before proceeding. This PR aims to push that conversation forward with some test cases illustrating proposed usages. Although here I created aTransferable
object, in most cases there's no need for the caller to explicitly construct a Transferable, just pass thebytes | Path
Proposed use cases:
TransferSpec
into the initializer: