-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Assign runner jar the proper permissions #1169
Conversation
Files.setPosixFilePermissions(runnerJar, PosixFilePermissions.fromString("rw-rw-r--")); | ||
} catch (Exception e) { | ||
log.warn("Unable to set proper permissions on " + runnerJar); | ||
} |
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.
Apparently, there's a good chance you will end with an UnsupportedOperationException
on Windows.
Maybe, we should just use the File
API? It brings us back in time but at least we will be able to make it work on Windows too.
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.
If this fails on Windows, then yes, I'll look into the File API stuff :)
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.
Rather than patching the permissions after the fact, wouldn't it be better to create the file with the right permissions upfront?
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.
When debugging I saw that the ZipFileSystem thing creates it with the correct permissions, but when that FS is closed, the permissions were being changed.
So I'm not really sure what's going and I didn't want to do something too intrusive this close to launch.
What you propose is obviously the better solution and I'll check and see if I can find a proper (and non intrusive fix)
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 swapped the PosixFilePermissions
with a simple setReadable
for the time being. I don't change the writable permissions since that would make it writable for all users (due to the terrible File API...). WDYT?
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.
fair enough!
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 was just reading Pro Java 7 NIO.2
and although it's obviously possible to get the proper permissions for both Posix and ACL style permissions, I think that it's overkill for this use case. So I propose we leave the PR as it is for the time being :)
987962e
to
14c382e
Compare
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.
Let's wait for CI and merge.
Merged. Good catch. |
Awesome, thanks! |
…1169) JBang now for known shrinkwrap resolution exception prints error message that contains custom repo list + cause chain on ouptut without having to use `--verbose` to see it.
Without this fix I was seeing the following on my path:
ls -l target/
which makes it harder to include
example-1.0.0.Alpha1-SNAPSHOT-runner.jar
in a Docker image that extends fromfabric8/java-jboss-openjdk8-jdk
.This fix sets read permissions for all users to
example-1.0.0.Alpha1-SNAPSHOT-runner.jar
.The one thing I don't know is if the fix will work on Windows (although even if it doesn't it shouldn't change anything)