feat: Add support for custom named log file names for log-rotate plugin #9619
Conversation
…in(supersedes 9210) Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
|
Please fix the ci |
monkeyDluffy6017
added
the
wait for update
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
approve ci |
|
@monkeyDluffy6017 Please approve CI |
|
Could you add |
| max_kept: 1 | ||
| enable_compression: true | ||
| --- config | ||
| location /t { | ||
| content_by_lua_block { | ||
| ngx.sleep(3.5) | ||
| ngx.sleep(2.5) |
There was a problem hiding this comment.
why is this change needed?
There was a problem hiding this comment.
The tests are flaky and fail intermittently and with this timeout configuration they stopped failing.
| @@ -105,6 +106,7 @@ start xxxxxx | |||
| --- config | |||
| location /t { | |||
| content_by_lua_block { | |||
| local io = require("io") | |||
There was a problem hiding this comment.
I think you will have to revert these changes.
There was a problem hiding this comment.
It fails lint check when I try to use global "io"
There was a problem hiding this comment.
There was a problem hiding this comment.
You have imported "io" but haven't used it.
There was a problem hiding this comment.
I have used it below in the for loop
There was a problem hiding this comment.
Strange, I don't see any other additions except importing "io" module in to this file 🤔
|
@Revolyssup please make the ci happy |
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
The failing tests are unrelated to this change :( |
apisix/plugins/log-rotate.lua
Outdated
| local ok, err = os_rename(log.file, new_file) | ||
| local filename = log.file | ||
| -- create the file if it does not exist | ||
| local exists = lfs.attributes(filename, "mode") == "file" |
There was a problem hiding this comment.
We have a funciton file_exists already
There was a problem hiding this comment.
@Revolyssup please check this
There was a problem hiding this comment.
okay I'll replace it with that function
There was a problem hiding this comment.
@monkeyDluffy6017 made the change
apisix/plugins/log-rotate.lua
Outdated
| if not exists then | ||
| local file = io_open(filename, "w") | ||
| if file then | ||
| file:close() | ||
| end | ||
| end |
There was a problem hiding this comment.
Why do you create a new file here, i think it's better to record an error log and exit the function
There was a problem hiding this comment.
Is it not desirable to automatically create the log file if it does not exist? @monkeyDluffy6017
There was a problem hiding this comment.
@Revolyssup please check this
There was a problem hiding this comment.
@monkeyDluffy6017 So should I error out when the file doesn't exist or create the file as a fallback?
There was a problem hiding this comment.
Is it necessay to add this check?
The original logic looked fine, os.rename would log when it failed and then skip
There was a problem hiding this comment.
@monkeyDluffy6017 removed this check
apisix/plugins/log-rotate.lua
Outdated
| local DEFAULT_ACCESS_LOG_FILENAME = "access.log" | ||
| local DEFAULT_ERROR_LOG_FILENAME = "error.log" | ||
| local DEFAULT_ACCESS_LOG_FILENAME = "logs/access.log" | ||
| local DEFAULT_ERROR_LOG_FILENAME = "logs/error.log" |
There was a problem hiding this comment.
I think it's better not to change this, the variable name is FILENAME, it should not include the file path
There was a problem hiding this comment.
Then I think we can change the variable name because we do require a path which currently is hardcoded to logs/access.log with hardcoding at two different places. I am changing the variable name to more appropriate DEFAULT_*_LOG_PATH @monkeyDluffy6017
There was a problem hiding this comment.
Yes I had changed the variable name.
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
|
Please make the ci pass |
|
I will close this pr becase it's fixed by #9749 |
Description
Supersedes #9210
Fixes #9061
Prior to this PR, the path to access and error log passed in config is not respected and path is hardcoded to logs/error.log and logs/access.log. This PR fixes that as well.
Checklist