Adding a check to test if Tar utility is present in the target container #974
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
/assign @yue9944882 |
There was a problem hiding this comment.
thanks for fixing this! i presume this makes the original function no-op when the tar is not present, which can be sometimes even more confusing for the users to troubleshoot. how do you think about declaring a checked exception like CopyNotSupportedException?
| if (!f.isDirectory() && !f.mkdirs()) { | ||
| throw new IOException("create directory failed: " + f); | ||
| // Test that 'tar' is present in the container? | ||
| if (isTarPresentInContainer(namespace, pod, container)) { |
There was a problem hiding this comment.
| if (isTarPresentInContainer(namespace, pod, container)) { | |
| if (!isTarPresentInContainer(namespace, pod, container)) { | |
| // do something | |
| } |
nit: plz reverse the test for a cleaner indent?
There was a problem hiding this comment.
I was originally thinking of exception but wasn't sure of the name. I have created an exception CopyNotSupportedException in io.kubernetes.client.util.exception package. Hope that is right place for it. I have also flipped the if check. Also added the CopyNotSupportedException in respective calling methods.
k8s-ci-robot
added
the
lgtm
yue9944882
added
the
tide/merge-method-squash
|
Thanks for the PR Ultimately, we may want to use ephemeral containers when they become more mature. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, sarveshkaushal The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Right now copy method throws runtime error if Tar is not present in the target container. Added a check for it before proceeding with copying process.
Note: This will only work for the containers which has POSIX based underlying image