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

feat(#501): enables to copy file from cluster into local container #725

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

buschNT
Copy link
Collaborator

@buschNT buschNT commented Oct 23, 2024

  • add --file-from flag to run command
  • copies file/directory from cluster using tar
  • writes tar into local container

- add --file-from flag to run command
- copies file/directory from cluster using tar
- writes tar into local container
- remove del line, not sure why this was an issue
MSG = (
"Invalid argument format. Please provide the file "
"in the format 'type/name:source:destination' or "
"'type/name/container-name:source:destination'."
Copy link

@crkurz crkurz Oct 24, 2024

Choose a reason for hiding this comment

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

Could we make destination optional? (I would suspect in most of the cases users will want the file to land in the same place it was copied from.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree. Good point.

Copy link

Choose a reason for hiding this comment

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

First of all thank you veeeeery much for this PR! This will reduce the number of things which otherwise need to be done in some wrapper script!

pls allow me to add some general questions/comments:

  • re parameter name « —file-from »: what would be the parameter name/syntax for future features like « copy all mounted files » (no more need to list out all the different dirs; or parameter name/syntax for the future feature « live file/folder sync between pod and container », which is needed e.g. to handle externally rotated cert files or for shared mounts between pod containers. - For now the background of my question is just to make sure that there won’t be any parameter name/syntax conflict for those future features.
  • by what is tar file size constrained? (in our case we are talking about tiny files, so we should be fine.)
  • does it make sense to allow batching of these tar runs? (only if they have significant overhead) - in our case we have to copy about 10 files/paths for a single container
  • can we make destination file/dir param optional? usually it will be same as source file/dir ?

If I can help with more details, I’d more than happy to do so. Please feel free to reach out to me on email or phone any time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree to most of the mentioned points, however, cannot answer all, especially the first point.

by what is tar file size constrained? (in our case we are talking about tiny files, so we should be fine.)

@SteinRobert brought up the same point. I will see how this can be implemented. We thought that a limit of around 50MB max might be good. I personally only transfer files of some KB, thus I don't have any feeling for a reasonable limit for other use cases.

does it make sense to allow batching of these tar runs?

I guess. I will check it out. This would also reduce the count of connections etc and might play nice together with the transfer limit.

can we make destination file/dir param optional? usually it will be same as source file/dir ?

Yes.

- implemented feedback from PR
- add tests for parse_file_from
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.

2 participants