-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Create temp files in a temp directory #450
Create temp files in a temp directory #450
Conversation
PR LGTM, does it work on Mac? (I can test Windows at home) |
@@ -168,6 +170,14 @@ private String extractClassPathResourceToTempLocation(final String hostPath) { | |||
return tmpLocation.getAbsolutePath(); | |||
} | |||
|
|||
private File createTempDirectory() { | |||
try { | |||
return Files.createTempDirectory(Paths.get(System.getProperty("java.io.tmpdir")), TESTCONTAINERS_TMP_DIR_PREFIX).toFile(); |
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.
Is there a reason you are not using createTempDirectory(String prefix, FileAttribute<?>... attrs)
? I assume this should have the same effect.
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.
Yes, you are right, it is the same.
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.
Do you want to change the method call, or do we have to specifically cater for macOS after all?
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 would like to change the method call and change temporary dir for macOS.
What do you think about the following method?
private File createTempDirectory() {
try {
if (SystemUtils.IS_OS_MAC) {
return Files.createTempDirectory(Paths.get("/tmp"), TESTCONTAINERS_TMP_DIR_PREFIX).toFile();
}
return Files.createTempDirectory(TESTCONTAINERS_TMP_DIR_PREFIX).toFile();
} catch (IOException e) {
return new File(TESTCONTAINERS_TMP_DIR_PREFIX + Base58.randomString(5));
}
}
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 which case would the code inside the catch
block be desired behavior I wonder?
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.
For example user runs java with params -Djava.io.tmpdir=<folder>
on linux/windows.
In cases, the folder does not exist we will get IOException.
I think we could try to create a temp folder in the current folder as it was before
@kiview Looks like we can not use Taking to account documentation https://docs.docker.com/docker-for-mac/osxfs/#access-control
Looks like we should use |
There's also this Windows specific line: I would prefer to have the platform specific branches in a single place (if possible). |
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.
While I like the change, it's important to understand that AFAIR it will not work with docker-machine on Mac, for instance. Or at least it didn't work. By default, docker-machine will not map /tmp volume and some manual configuration is required. It was also the case for Docker for Mac, but then they added /tmp and /private/tmp to the list of mounts. Before accepting this PR, we have to check it against different versions of docker-machine and Docker for Mac to make sure that we will not introduce a regression. Or, even if we do so, we should mention it somewhere that the minimum d4m/Docker Machine version is that now :)
Yes, I agree with @bsideup - unfortunately there are quite a few permutations we need to make sure we have covered. I think specifying a new 'minimum version' for Mac clients wouldn't be too evil, though. In all likelihood, developer machines will be receiving fairly regular/automatic updates, so it's not as much of a problem as requiring a recent version of Docker on Linux CI servers, for example. I'll try and do a bit of Mac testing for this tomorrow. |
I quickly tested this PR on my Windows box (Windows 10, Docker for Windows 17.06.2-ce-win27) and the After rebasing the PR on master, the tests run fine on Windows and the temp file will be created in So functionality looks fine on Docker for Windows. |
5117e1a
to
a0ff477
Compare
@kiview I rebase with the master. Also, I added the specific implementation for mac. Unfortunately, I'm not currently able to run the build on Mac or Windows. I'll run build on Mac tomorrow(as soon as possible). |
Build passed on Mac |
return Files.createTempDirectory(Paths.get(OS_MAC_TMP_DIR), TESTCONTAINERS_TMP_DIR_PREFIX).toFile(); | ||
} | ||
return Files.createTempDirectory(TESTCONTAINERS_TMP_DIR_PREFIX).toFile(); | ||
} catch (IOException e) { |
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.
Can you please explain the conditions and circumstances, under which you expect this Exception
to happen and how this return value solves the problem?
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.
For example user runs java with params -Djava.io.tmpdir=<folder>
on linux/windows.
In cases, the folder does not exist we will get IOException.
I think we could try to create a temp folder in the current folder as it was before.
Is it make sense?
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.
Okay, sounds good.
@@ -33,6 +33,9 @@ | |||
@Slf4j | |||
public class MountableFile implements Transferable { | |||
|
|||
private static final String TESTCONTAINERS_TMP_DIR_PREFIX = ".testcontainers-tmp-"; | |||
public static final String OS_MAC_TMP_DIR = "/tmp"; |
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.
Does this need to be public?
Anyone up for regression testing this on older Docker for Mac versions? |
Is
on Yosemite (10.10.5) old enough? |
If it's helpful,
On the banadiga:create-temp-files-in-atemp-dorectory repo/branch it hangs in the same place. |
So support might be already broken for |
Perhaps, though I'm using testcontainers-java version 1.4.2 just fine. |
@dbyron0 seems like there must be another issue in between 1.4.2 and master - sorry 😞 I'm going to do some compatibility testing this evening with old versions of docker toolbox, running on OS X Sierra. |
Well, turns out I lied...at least a little. I'm using 1.4.2 in a project of mine and it does work fine.
where src/testcontainers-java/core/target/surefire-reports/2017-09-27T13-21-48_396.dumpstream has:
|
Ah, @dbyron0 that looks like netty/netty#6837 popping up again 😬 . We explicitly made sure that it would not arise in normal usage on OS X <10.12, but it seems that in our own build process at least the affected code path is being activated. I'm afraid this means that testcontainers development on OS X requires 10.12+ now. |
Thanks for working that out. Looks like it's time for an upgrade, or if I'm very lucky, a new machine. |
@rnorth Do you think we can risk merging and dropping support for older versions? |
Re development of testcontainers not being possible on older versions of OS X - I'm OK with that. By way of an additional regression test for this feature, I've run it against a OS X Docker Machine using a boot2docker v1.12.1 ISO, which was released in August 2016. I'd like to try and test with a similarly old version of the docker-machine tool itself, but this will be time consuming. Will set that in motion, though. |
Bump, any news on this? |
Sorry for the delay. I've tested with docker-machine 0.8.2 with the 1.12.1 boot2docker ISO (dating back to September 2016) and it's fine. As such, LGTM :) |
Have rebased, resolved merge conflicts, and merged into master. Thanks @banadiga ! |
Bumps [influxdb-java](https://github.com/influxdata/influxdb-java) from 2.10 to 2.14. <details> <summary>Changelog</summary> *Sourced from [influxdb-java's changelog](https://github.com/influxdata/influxdb-java/blob/master/CHANGELOG.md).* > ## 2.14 [2018-10-12] > > ### Fixes > > - Fixed chunked query exception handling [Issue #523](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/523) > - Memory leak in StringBuilder cache for Point.lineprotocol() [Issue #526](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/521) > > ## 2.13 [2018-09-12] > > ### Fixes > - MessagePack queries: Exception during parsing InfluxDB version [macOS] [PR #487](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/487) > - The InfluxDBResultMapper is able to handle results with a different time precision [PR #501](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/501) > - UDP target host address is cached [PR #502](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/502) > - Error messages from server not parsed correctly when using msgpack [PR #506](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/506) > - Response body must be closed properly in case of JSON response [PR #514](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/514) > - Time is serialized not consistently in MsgPack and Json, missing millis and nanos in MsgPack[PR #517](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/517) > > ### Features > > - Support for Basic Authentication [PR #492](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/492) > - Added possibility to reuse client as a core part of [influxdb-java-reactive](https://github.com/bonitoo-io/influxdb-java-reactive) client [PR #493](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/493) > - Retry capability for writing of BatchPoints [PR #503](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/503) > - Added `BiConsumer` with capability to discontinue a streaming query [Issue #515](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/515) > - Added `onComplete` action that is invoked after successfully end of streaming query [Issue #515](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/515) > > ## 2.12 [2018-07-31] > > ### Fixes > > - Remove code which checks for unsupported influxdb versions [PR #474](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/474) > - Unpredictable errors when OkHttpClient.Builder instance is reused [PR #478](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/478) > > ### Features > > - Support for MessagePack [PR #471](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/471) > - Cache version per influxdb instance and reduce ping() calls for every query call [PR #472](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/472) > - FAQ list for influxdb-java [PR #475](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/475) > > ### Improvements > > - Test: Unit test to ensure tags should be sorted by key in line protocol (to reduce db server overheads) [PR #476](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/476) > > ## 2.11 [2018-07-02] > > ### Features > > - Allow write precision of TimeUnit other than Nanoseconds [PR #321](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/321) > - Support dynamic measurement name in InfluxDBResultMapper [PR #423](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/423) > - Debug mode which allows HTTP requests being sent to the database to be logged [PR #450](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/450) > - Fix problem of connecting to the influx api with URL which does not points to the url root (e.g. localhots:80/influx-api/) [PR #400] (https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/400) ></table> ... (truncated) </details> <details> <summary>Commits</summary> - [`91d0f09`](influxdata/influxdb-java@91d0f09) [maven-release-plugin] prepare release influxdb-java-2.14 - [`8ffaeb9`](influxdata/influxdb-java@8ffaeb9) Revert "[maven-release-plugin] prepare release influxdb-java-2.14" - [`2781da2`](influxdata/influxdb-java@2781da2) [maven-release-plugin] prepare release influxdb-java-2.14 - [`19c69ed`](influxdata/influxdb-java@19c69ed) [maven-release-plugin] prepare for next development iteration - [`c6d7f25`](influxdata/influxdb-java@c6d7f25) [maven-release-plugin] prepare release influxdb-java-2.14 - [`2f4c594`](influxdata/influxdb-java@2f4c594) Merge pull request [#531](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/531) from heshengbang/master - [`f653e62`](influxdata/influxdb-java@f653e62) Easy to use try-with-resources, add README.md - [`c7be9b0`](influxdata/influxdb-java@c7be9b0) Easy to use try-with-resources - [`4590d18`](influxdata/influxdb-java@4590d18) - added automated SNAPSHOT publishing to Maven Central repository - [`ce65a41`](influxdata/influxdb-java@ce65a41) - added automated SNAPSHOT publishing to Maven Central repository - Additional commits viewable in [compare view](influxdata/influxdb-java@influxdb-java-2.10...influxdb-java-2.14) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=org.influxdb:influxdb-java&package-manager=gradle&previous-version=2.10&new-version=2.14)](https://dependabot.com/compatibility-score.html?dependency-name=org.influxdb:influxdb-java&package-manager=gradle&previous-version=2.10&new-version=2.14) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- **Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit. You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com). <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in the `.dependabot/config.yml` file in this repo: - Update frequency (including time of day and day of week) - Automerge options (never/patch/minor, and dev/runtime dependencies) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) Finally, you can contact us by mentioning @dependabot. </details>
The base idea is to create folder
.testcontainers.tmp.something
in a temporary directory.Fix for #423