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

Read boot-env using io/resource and io/reader #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

larkery
Copy link

@larkery larkery commented Apr 17, 2018

Previously used io/file with io/resource, which is not safe when the resource is not a file.

Something like this should work?

@weavejester
Copy link
Owner

Thanks for the patch. Can you wrap the commit message at 72 characters and add a reference to the issue being solved, e.g. something like:

Read boot-env using io/resource and io/reader

Using io/file with io/resource is not safe when the resource is not a
file, so would cause uberjars with environ to fail.

Fixes: #80

Previously, classpath resources were assumed to be files and passed to
io/file. This failed when the resource was within a jar. The fix is to
use io/reader instead, which works on resources within or without
jars.

Fixes: weavejester#80
@larkery
Copy link
Author

larkery commented Apr 20, 2018

I've twiddled the commit a bit to reformat the message and put in a test that .boot-env is read off the classpath as a resource.

@weavejester
Copy link
Owner

weavejester commented Apr 20, 2018

Thanks! However I think the commit message should make it clear that including .boot-env in a jar is not the intended purpose of the file. So perhaps:

Read boot-env using io/resource and io/reader

Ensures that Environ doesn't crash if .boot-env is accidentally included
in a jar file on the classpath.

Fixes: #80

We don't need to explain exactly how it was fixed, since I think that's self-explanatory for the diff.

Both .boot-env and .lein-env are intended to be temporary files to facilitate the transfer of data from the build script into the running process.

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