Don't fallback on tmp if it's not writable #158
Conversation
tombruijn
force-pushed
the
log_file_path-improvements
branch
from
d21cebf
to
6bc3bdf
Compare
|
@jeffkreeftmeijer and @thijsc please re-review I now check the Changed $stderr to $stdout, because that's what we use most of the time. I'll squash these commits after approval I'll send in another PR for changes to |
| end | ||
| rescue Errno::ENOENT |
There was a problem hiding this comment.
Would it make sense to reinstate the rescue here to make sure we print a useful error message when we can't write to the file for any other reason than we've just checked?
There was a problem hiding this comment.
I removed it because I'm not quite sure what can cause an Errno::ENOENT to be thrown here.
File.writable? doesn't throw it for a file that doesn't exist or is not writable.
There was a problem hiding this comment.
You're right. Errno::ENOENT doesn't seem to get raised when calling#writable?, so I'm not sure why this was here in the first place. Could we catch this when trying to write to the file?
There was a problem hiding this comment.
@jeffkreeftmeijer that's what PR #160 is for 😄
| end | ||
| rescue Errno::ENOENT |
| @@ -90,13 +91,18 @@ def []=(key, value) | |||
|
|
|||
| def log_file_path | |||
| path = config_hash[:log_path] || root_path | |||
| if path && File.writable?(path) | |||
| File.join(File.realpath(path), 'appsignal.log') | |||
| return File.join(path, 'appsignal.log') if path && File.writable?(path) | |||
There was a problem hiding this comment.
The File.realpath call is essential because it allows us to break out Capistrano style release directories to shared directories. Without this an agent that survives enough deploys to live through a cleanup dies.
There was a problem hiding this comment.
Ah right, readded the case and added some tests for it too :)
|
Ready for re-review Note: I'd like to squash these commits on merge, no need to remove the |
|
Indeed. Especially in the gem I only want commits in the history that are actually released. |
Currently if the set log path, or the project path, are not writable we always fallback on the `/tmp/appsignal.log` fallback path. This breaks if the fallback path is also not writable. This change adds a permission check on the /tmp fallback path and returns `nil` if no suitable log path can be found. Update test to handle more cases.
tombruijn
force-pushed
the
log_file_path-improvements
branch
from
9228bb6
to
8e8de22
Compare
Currently if the set log path, or the project path, are not writable we
always fallback on the
/tmp/appsignal.logfallback path. This breaksif the fallback path is also not writable.
This change adds a permission check on the /tmp fallback path and
returns
nilif no suitable log path can be found.Update test to handle more cases.
This should fix problem 1 in #153