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

Introduce an abstraction over files and classpath resources. #279

Merged
merged 2 commits into from
Mar 12, 2017

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Jan 22, 2017

This encapsulates all the complexity of generating a path that the Docker daemon is about to create a volume mount for.

This should resolve a general problem with spaces in paths, which was seen in one particular form as #263.

Additional tests for the specific problem in #263 will be added shortly.

@rnorth rnorth modified the milestones: 1.1.8, 1.1.9 Jan 22, 2017
private final String path;
private String resolvedPath = null;

private MountableFile(final String path) {
Copy link
Member

Choose a reason for hiding this comment

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

@RequiredArgsConstructor(access = lombok.AccessLevel.PRIVATE)

*/
public String getMountablePath() {

// Don't recompute if already resolved
Copy link
Member

Choose a reason for hiding this comment

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

how about @Getter(lazy = true) ?

}
String internalPath = hostPath.replaceAll("[^!]*!/", "");

try (JarFile jarFile = new JarFile(urldecodedJarPath)) {
Copy link
Member

Choose a reason for hiding this comment

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

starting from Java 7 we can use Jar (aka zip) FileSystem provider:
http://docs.oracle.com/javase/7/docs/technotes/guides/io/fsp/zipfilesystemprovider.html

it also might contain path escaping OOTB (not sure here, have to re-check)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah great - I missed this. This should make things quite a bit easier. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this, but it ended up not being any clearer, so I kept as-is!

public void forClasspathResource() throws Exception {
final MountableFile mountableFile = MountableFile.forClasspathResource("mappable-resource/test-resource.txt");

performChecks(mountableFile);
Copy link
Member

Choose a reason for hiding this comment

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

how about @Parameterized here?

Copy link
Member Author

Choose a reason for hiding this comment

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

3 out of 5 test methods could be joined using Parameterized, but there would still have been two quite different test cases. In the end I've left as it was - it's fairly obvious what's going on, and we'd not have reduced the amount of code much by using Parameterized tests here.

@bsideup
Copy link
Member

bsideup commented Feb 17, 2017

@rnorth how is the progress? Do you need my help to deliver it?

This encapsulates all the complexity of generating a path that the Docker daemon is about to create a volume mount for.

This should resolve a general problem with spaces in paths, which was seen in one particular form as #263.

Additional tests for the specific problem in #263 will be added shortly.
@rnorth rnorth changed the title WIP: Introduce an abstraction over files and classpath resources. Introduce an abstraction over files and classpath resources. Mar 12, 2017
@rnorth rnorth merged commit 4caddcc into master Mar 12, 2017
@rnorth rnorth deleted the 263-fix-paths-with-spaces branch March 12, 2017 20:47
rnorth added a commit that referenced this pull request Mar 12, 2017
## [1.2.0] - 2017-03-12
### Fixed
- Fix various escaping issues that may arise when paths contain spaces (#263, #279)
- General documentation fixes/improvements (#300, #303, #304)
- Improve reliability of `ResourceReaper` when there are a large number of containers returned by `docker ps -a` (#295)

### Changed
- Support Docker for Windows via TCP socket connection (#291, #297, #309). _Note that Docker Compose is not yet supported under Docker for Windows (see #306)
- Expose `docker-java`'s `CreateContainerCmd` API for low-level container tweaking (#301)
- Shade `org.newsclub` and Guava dependencies (#299, #292)
- Add `org.testcontainers` label to all containers created by Testcontainers (#294)
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