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

Also allow copying transferables using withCopyFileToContainer #5180

Closed
wants to merge 1 commit into from
Closed

Also allow copying transferables using withCopyFileToContainer #5180

wants to merge 1 commit into from

Conversation

dajudge
Copy link
Member

@dajudge dajudge commented Mar 24, 2022

Fixes #3814

@dajudge dajudge self-assigned this Mar 24, 2022
@kiview
Copy link
Member

kiview commented Mar 24, 2022

Just referencing #3815 here, so we keep the connection (and maybe we can integrate the test).

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

I think keeping the ordering semantics is an improvement over #3814. So let's maybe include an explicit test for Transferable as well?

@@ -1284,15 +1279,97 @@ public SELF withWorkingDirectory(String workDir) {
return self();
}

private interface CopyFileToContainerWrapper<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we find an interface name with more meaning for this? One might argue, that Transferable could have been a good abstraction already (we can transfer bytes of a file into the container), but of course, this is already used differently in our codebase now.

Maybe CopyFileToContainerSource ? If this is reflected in some parameters being renamed from wrapper to source, this should also help readability.

@@ -174,6 +175,15 @@ default SELF withFileSystemBind(String hostPath, String containerPath) {
*/
SELF withCopyFileToContainer(MountableFile mountableFile, String containerPath);

/**
* Set the file to be copied before starting a created container
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Set the file to be copied before starting a created container
* Set the Transferable to be copied before starting a created container

It is not really a file, or?

@dajudge
Copy link
Member Author

dajudge commented Mar 24, 2022

I think keeping the ordering semantics is an improvement over #3814. So let's maybe include an explicit test for Transferable as well?

Yeah there needs to be a test for sure - just wanted to reach some consensus that we're going in that direction before spending the time on it.

@dajudge dajudge closed this by deleting the head repository Dec 13, 2023
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.

Allow to create a container file from a Transferable
2 participants