clean tmp file after upload #848

fengxx · 2017-05-23T12:45:17Z

CopyArchiveToContainerCmdImpl kept temp file on disk, need remove it after upload

This change is

codecov-io · 2017-05-23T13:13:38Z

Codecov Report

Merging #848 into master will decrease coverage by 0.03%.
The diff coverage is 54.54%.

@@            Coverage Diff            @@
##           master    #848      +/-   ##
=========================================
- Coverage   71.63%   71.6%   -0.04%     
=========================================
  Files         306     306              
  Lines        6643    6649       +6     
  Branches      500     501       +1     
=========================================
+ Hits         4759    4761       +2     
- Misses       1591    1595       +4     
  Partials      293     293

Impacted Files	Coverage Δ
...va/core/command/CopyArchiveToContainerCmdImpl.java	`65.3% <54.54%> (-4.47%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08ac9b6...a815be9. Read the comment docs.

KostyaSha · 2017-05-23T14:27:34Z

src/main/java/com/github/dockerjava/core/command/CopyArchiveToContainerCmdImpl.java

+            try {
+                toUpload = Files.createTempFile("docker-java", ".tar.gz");
+                CompressArchiveUtil.tar(Paths.get(hostResource), toUpload, true, dirChildrenOnly);
+                this.tarInputStream = Files.newInputStream(toUpload);


should this be wrapped with try-with-resources or it closed in somewhere else place?

KostyaSha · 2017-05-23T15:14:09Z

src/main/java/com/github/dockerjava/core/command/CopyArchiveToContainerCmdImpl.java

+                CompressArchiveUtil.tar(Paths.get(hostResource), toUpload, true, dirChildrenOnly);
+            } catch (IOException createFileIOException) {
+                throw new DockerClientException("Unable to perform tar on host resource " + this.hostResource, createFileIOException);
+            }


should it have finally also here with toUpload remove?

added remove in catch clause

KostyaSha · 2017-05-23T15:14:57Z

src/main/java/com/github/dockerjava/core/command/CopyArchiveToContainerCmdImpl.java

-                throw new DockerClientException("Unable to perform tar on host resource " + this.hostResource, e);
+                throw new DockerClientException("Unable to read temp file " + toUpload.toFile().getAbsolutePath(), e);
+            } finally {
+                toUpload.toFile().delete();


please place comment // remove tmp dir

KostyaSha · 2017-05-23T16:33:16Z

src/main/java/com/github/dockerjava/core/command/CopyArchiveToContainerCmdImpl.java

+                }
+                throw new DockerClientException("Unable to perform tar on host resource " + this.hostResource, createFileIOException);
+            }
+            try (InputStream uploadStream = Files.newInputStream(toUpload)) {


add new line before for better readability and squash commits (to keep contribution in GH)

KostyaSha

small change, pr build and will merge

fengxx

Thanks for review, fixed

fengxx · 2017-05-23T15:35:30Z

src/main/java/com/github/dockerjava/core/command/CopyArchiveToContainerCmdImpl.java

+                CompressArchiveUtil.tar(Paths.get(hostResource), toUpload, true, dirChildrenOnly);
+            } catch (IOException createFileIOException) {
+                throw new DockerClientException("Unable to perform tar on host resource " + this.hostResource, createFileIOException);
+            }


added remove in catch clause

KostyaSha · 2017-05-24T08:10:41Z

src/main/java/com/github/dockerjava/core/command/CopyArchiveToContainerCmdImpl.java

            } catch (IOException e) {
-                throw new DockerClientException("Unable to perform tar on host resource " + this.hostResource, e);
+                throw new DockerClientException(
+                        "Unable to read temp file " + toUpload.toFile().getAbsolutePath(), e);


120..140 width is allowed to use :)

thanks for patience, formatted code use 140

KostyaSha · 2017-05-24T09:47:46Z

.idea/codeStyleSettings.xml

      <value>
        <option name="CLASS_COUNT_TO_USE_IMPORT_ON_DEMAND" value="9999" />
        <option name="NAMES_COUNT_TO_USE_IMPORT_ON_DEMAND" value="9999" />
+        <option name="RIGHT_MARGIN" value="140" />


https://github.com/docker-java/docker-java/blob/master/src/test/resources/checkstyle/checkstyle-config.xml#L91-L92
thanks. Added file only some time before, was not fully filled.

clean tmp file after upload

KostyaSha reviewed May 23, 2017

View reviewed changes

fengxx force-pushed the feature/clean_tmp branch 2 times, most recently from 75fe996 to e1004d5 Compare May 23, 2017 15:13

KostyaSha reviewed May 23, 2017

View reviewed changes

fengxx force-pushed the feature/clean_tmp branch 2 times, most recently from 989633a to 37525ee Compare May 23, 2017 15:46

KostyaSha reviewed May 23, 2017

View reviewed changes

KostyaSha requested changes May 23, 2017

View reviewed changes

fengxx force-pushed the feature/clean_tmp branch from 37525ee to a815be9 Compare May 24, 2017 02:20

fengxx commented May 24, 2017

View reviewed changes

KostyaSha reviewed May 24, 2017

View reviewed changes

clean tmp file after upload

a9b78cc

fengxx force-pushed the feature/clean_tmp branch from a815be9 to a9b78cc Compare May 24, 2017 09:32

KostyaSha reviewed May 24, 2017

View reviewed changes

KostyaSha added this to the 3.0.11 milestone May 31, 2017

KostyaSha approved these changes May 31, 2017

View reviewed changes

KostyaSha merged commit 51bd54a into docker-java:master May 31, 2017

panuse pushed a commit to TuKangTech/docker-java that referenced this pull request Aug 20, 2017

Merge pull request docker-java#848 from fengxx/feature/clean_tmp

065d025

clean tmp file after upload

clean tmp file after upload #848

clean tmp file after upload #848

Uh oh!

Conversation

fengxx commented May 23, 2017 • edited by marcuslinke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaSha left a comment

Choose a reason for hiding this comment

Uh oh!

fengxx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaSha May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaSha May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fengxx commented May 23, 2017 •

edited by marcuslinke

Loading

codecov-io commented May 23, 2017 •

edited

Loading

KostyaSha May 24, 2017 •

edited

Loading

KostyaSha May 24, 2017 •

edited

Loading