sevdokimov / log-viewer

Web UI for viewing logs
Apache License 2.0
477 stars 105 forks source link

feat: add support for gzip archive files #157

Closed mhewedy closed 1 year ago

mhewedy commented 1 year ago

This PR is to support gz files as asked in #132 and #131

mhewedy commented 1 year ago

I think PR is not ready yet ...

as while I am doing a couple of manual tests, I faced this error :

image

However, the file is still there and I can cat it normally.. this happens on Linux and not windows, on windows it works just fine.

Any ideas?

mhewedy commented 1 year ago

I figured out the issue when I checked the WS response (server log was silient though)...

the issue was related to logs path config... I was customized it to look into some specified folder in '/mnt/logs'

So to fix the issue, I added

{
  path: "/tmp/**/*"
}

in my config file as well to logs property.

So it has nothing to do with PR, unless the user should be aware of the paths config in such case!

sevdokimov commented 1 year ago

Thanks! Good idea. But unpacking all log files automatically is not a safe operation, logs may take a lot of disk space. Let's add log-viewer.unpack-archive configuration property and disable it by default. The ideal solution is ask the user if he want to unpack log because it is not expected. Adding asking of the user is not a trivial fix, I'll implement it later.

sevdokimov commented 1 year ago

I think PR is not ready yet ...

as while I am doing a couple of manual tests, I faced this error :

image

However, the file is still there and I can cat it normally.. this happens on Linux and not windows, on windows it works just fine.

Any ideas?

Don't change Log.file property, this property is used for permission checks. You can add Log.realFile property, that should be initialized in Log.LogSnapshot.getChannel() if it's null

mhewedy commented 1 year ago

realFile

I searched the entire code base but didn't find a track for realFile.. can you elaborate more on this?

mhewedy commented 1 year ago

I've updated the PR by suggested comments:

  1. introduce log-viewer.unpack-archive
  2. check the gzip file extension instead
sevdokimov commented 1 year ago

realFile

I searched the entire code base but didn't find a track for realFile.. can you elaborate more on this?

I mean you have to add Log.realFile to com.logviewer.data2.Log class. Now com.logviewer.data2.Log has file field, your code replace the value to the temporary file. I propose to keep the original file name in Log.file , but put unpacked file to Log.realFile. I can add it after merge.

mhewedy commented 1 year ago

BTW, I did other changes as well to suit needs https://github.com/sevdokimov/log-viewer/compare/master...mhewedy:log-viewer:develop

Let me know if any interesting feature that I can contribute back

sevdokimov commented 1 year ago

I added the following changes after merge: 1.) renamed the configuration property to log-viewer.unpack-gz-archives 2.) Fixed checking of log visibility: no error if .gz file is visible, but the unpacked '/tmp/log-viewer*.tml' is not. 3.) Added the details error message when the user is trying to open .gz file with disabled log-viewer.unpack-gz-archives 4.) added tests

mhewedy commented 1 year ago

Good, Thanks 🌹