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

MountableFile fails when path contains + (plus) #664

Closed
bsideup opened this issue Apr 26, 2018 · 8 comments
Closed

MountableFile fails when path contains + (plus) #664

bsideup opened this issue Apr 26, 2018 · 8 comments
Assignees
Labels
Milestone

Comments

@bsideup
Copy link
Member

bsideup commented Apr 26, 2018

As seen in elastic/apm-agent-java#57, file mounting doesn't work when path containers + (plus) character. It got replaced with (space)

@bsideup bsideup added this to the next milestone Apr 26, 2018
@bsideup bsideup self-assigned this Apr 26, 2018
@bsideup
Copy link
Member Author

bsideup commented Apr 26, 2018

@felixbarny I created an issue for that bug, the PR is also submitted.
If you feel comfortable with Jitpack you can even try it in your project with:

allprojects {
    repositories {
        // ...
        maven { url 'https://jitpack.io' }
   }
}
// ...
dependency 'com.github.testcontainers.testcontainers-java:testcontainers:b625e62ffe'

@felixbarny
Copy link

I was just about to point out the unencodeResourceURIToFilePath method.

There also seems to be a problem with the sources jar which made debugging a bit harder as line numbers don't match.

Does this also resolve the problem with withFileSystemBind? docker-java/docker-java@1952fa6 seems to be already released in 3.1.0-rc-2.

@bsideup
Copy link
Member Author

bsideup commented Apr 26, 2018

@felixbarny yes, it should fix both methods.

There also seems to be a problem with the sources jar which made debugging a bit harder as line numbers don't match.

Oh wow! We're running Delombok to make the sources look better (otherwise IntelliJ compains) but apparently it breaks the debugging experience. Thanks, will check what we can do to make both work 👍

@felixbarny
Copy link

felixbarny commented Apr 30, 2018

Thx for the fix. I'd appreciate a release for this.

@bsideup
Copy link
Member Author

bsideup commented Apr 30, 2018

@felixbarny going to be released today :)

@bsideup
Copy link
Member Author

bsideup commented Apr 30, 2018

@felixbarny released as 1.7.2 :)

@felixbarny
Copy link

thx a lot!

@bsideup
Copy link
Member Author

bsideup commented Apr 30, 2018

@felixbarny I really hope this is the last & only blocker for Elastic APM 😅 took much longer than supposed to take to integrate, sorry for that 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants