Create temp files in a temp directory#450
Create temp files in a temp directory#450banadiga wants to merge 4 commits intotestcontainers:masterfrom
Conversation
|
PR LGTM, does it work on Mac? (I can test Windows at home) |
|
|
||
| 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.
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.
Yes, you are right, it is the same.
There was a problem hiding this comment.
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.
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.
In which case would the code inside the catch block be desired behavior I wonder?
There was a problem hiding this comment.
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). |
bsideup
left a comment
There was a problem hiding this comment.
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.
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.
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?
| public class MountableFile implements Transferable { | ||
|
|
||
| private static final String TESTCONTAINERS_TMP_DIR_PREFIX = ".testcontainers-tmp-"; | ||
| public static final String OS_MAC_TMP_DIR = "/tmp"; |
|
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 /> [](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.somethingin a temporary directory.Fix for #423