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

Always use toAbsolutePath() for MountableFile #3514

Merged
merged 8 commits into from
Dec 11, 2020
Merged

Conversation

kiview
Copy link
Member

@kiview kiview commented Nov 23, 2020

forHostPath in MountableFile will now always use toAbsolutePath(), instead of using toURI() in certain cases.
Solves an issue on Windows that was introduced with the latest Docker Desktop on Windows update.

Fixes #3493.

@bsideup
Copy link
Member

bsideup commented Nov 23, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kiview
Copy link
Member Author

kiview commented Nov 23, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Fufusulier
Copy link

Hello,
is there a reason why this PR is still opened?
Thanks

@bsideup
Copy link
Member

bsideup commented Nov 26, 2020

@Fufusulier well, "Testcontainers isn't our primary job" is probably the best answer to "a reason why this PR is still opened" 😅

We're on it, just it takes time to handle everything. Stay tuned 👍

@Fufusulier
Copy link

Fufusulier commented Nov 26, 2020

@Fufusulier well, "Testcontainers isn't our primary job" is probably the best answer to "a reason why this PR is still opened" 😅

We're on it, just it takes time to handle everything. Stay tuned 👍

oooh ok, I thought it was. sorry :) I'm a bit impatient because I'm stuck :p

@bsideup
Copy link
Member

bsideup commented Nov 26, 2020

@Fufusulier no problem! We always take it as a compliment :)

We thought that the issue is in docker-java, but apparently it wasn't, so it takes a bit longer to fix it, but at least we identified the root cause and working on it 👍

@kiview
Copy link
Member Author

kiview commented Nov 30, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kiview
Copy link
Member Author

kiview commented Nov 30, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@bsideup
Copy link
Member

bsideup commented Dec 10, 2020

/azp run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bsideup bsideup added this to the 1.15.1 milestone Dec 11, 2020
@Fufusulier
Copy link

this feels good :p

@jetersen
Copy link
Contributor

jetersen commented Mar 12, 2021

This change sadly stopped accepting Paths from a class resource.
MountableFile does not accept a class to properly load resources from a reasonable class loader.

.withCopyFileToContainer(
    forHostPath(TestConstants.class.getResource("vaultTest_agent.hcl").getPath()),
    "/agent.hcl")

I needed to copy this method since it is marked private 😓

private String resolvePath() {
String result = getResourcePath();
// Special case for Windows
if (SystemUtils.IS_OS_WINDOWS && result.startsWith("/")) {
// Remove leading /
result = result.substring(1);
}
return result;
}

See https://github.com/jenkinsci/hashicorp-vault-plugin/pull/168/files#diff-410d6791498036e7b693cae373e4c4e1c1a76d3cdc26610d3112e95709b83ff0R66-R79

@bsideup
Copy link
Member

bsideup commented Mar 13, 2021

@jetersen any reason you're not using forClasspathResource? forHostPath with classpath worked by accident, and forClasspathResource is the API that should be used instead :)

@jetersen
Copy link
Contributor

jetersen commented Mar 13, 2021

forClasspathResource returns Resources are not found

MountableFile does not accept a class to properly load resources from a reasonable class loader.

As I said it using a wrong class loader.

java.lang.ExceptionInInitializerError
	at sun.misc.Unsafe.ensureClassInitialized(Native Method)
	at sun.reflect.UnsafeFieldAccessorFactory.newFieldAccessor(UnsafeFieldAccessorFactory.java:43)
	at sun.reflect.ReflectionFactory.newFieldAccessor(ReflectionFactory.java:156)
	at java.lang.reflect.Field.acquireFieldAccessor(Field.java:1088)
	at java.lang.reflect.Field.getFieldAccessor(Field.java:1069)
	at java.lang.reflect.Field.get(Field.java:393)
	at org.junit.runners.model.FrameworkField.get(FrameworkField.java:92)
	at org.junit.runners.model.TestClass.collectAnnotatedFieldValues(TestClass.java:249)
	at org.junit.runners.ParentRunner.classRules(ParentRunner.java:280)
	at org.junit.runners.ParentRunner.withClassRules(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.classBlock(ParentRunner.java:217)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:412)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:220)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:53)
Caused by: java.lang.IllegalArgumentException: Resource with path vaultTest_adminPolicy.hcl could not be found on any of these classloaders: [sun.misc.Launcher$AppClassLoader@18b4aac2]
	at org.testcontainers.utility.MountableFile.getClasspathResource(MountableFile.java:147)
	at org.testcontainers.utility.MountableFile.forClasspathResource(MountableFile.java:97)
	at org.testcontainers.utility.MountableFile.forClasspathResource(MountableFile.java:66)
	at com.datapipe.jenkins.vault.util.VaultTestUtil.createVaultContainer(VaultTestUtil.java:59)
	at com.datapipe.jenkins.vault.jcasc.secrets.VaultSecretSourceTest.<clinit>(VaultSecretSourceTest.java:50)
	... 17 more

@bsideup
Copy link
Member

bsideup commented Mar 13, 2021

@jetersen are you sure the problem is with the classloaders? You seem to be providing path vaultTest_adminPolicy.hcl, while most probably the actual path is something like com/example/tests/constants/vaultTest_adminPolicy.hcl (judging by the TestConstants class name). When you do TestConstants.class.getResource("vaultTest_agent.hcl"), it will append TestConstants' package to the resource path.

@jetersen
Copy link
Contributor

jetersen commented Mar 13, 2021

Right, that was not obvious for the method name and javadoc that I had to provide the full resource path as the variable is named resourceName and not resourcePath

@bsideup
Copy link
Member

bsideup commented Mar 13, 2021

I agree that the naming could be improved, ut it is similar to other classpath APIs (eg Spring's ClasspathResource) - you need to provide the full path, since there is no "current directory" when it comes to classpath :)

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

Successfully merging this pull request may close these issues.

Parsing error after upgrading docker version on Windows
4 participants