Revolyssup commented on code in PR #9619:
URL: https://github.com/apache/apisix/pull/9619#discussion_r1230446319
##########
apisix/plugins/log-rotate.lua:
##########
@@ -72,11 +74,11 @@ local function file_exists(path)
return file ~= nil
end
-
+-- get the log directory path
Review Comment:
added
##########
apisix/plugins/log-rotate.lua:
##########
@@ -101,15 +103,15 @@ local function get_log_path_info(file_type)
end
end
- return prefix .. "logs/", file_type
+ return prefix , file_type
end
local function tab_sort_comp(a, b)
return a > b
end
-
+-- scans the log directory and returns a sorted table of matching log files
Review Comment:
done
##########
apisix/plugins/log-rotate.lua:
##########
@@ -143,10 +157,18 @@ local function rename_file(log, date_str)
core.log.info("file exist: ", new_file)
return new_file
end
-
- local ok, err = os_rename(log.file, new_file)
Review Comment:
done
##########
apisix/plugins/log-rotate.lua:
##########
@@ -143,10 +157,18 @@ local function rename_file(log, date_str)
core.log.info("file exist: ", new_file)
return new_file
end
-
- 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"
Review Comment:
okay I'll replace it with that function
##########
apisix/plugins/log-rotate.lua:
##########
@@ -143,10 +157,18 @@ local function rename_file(log, date_str)
core.log.info("file exist: ", new_file)
return new_file
end
-
- 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"
+ if not exists then
+ local file = io_open(filename, "w")
+ if file then
+ file:close()
+ end
+ end
Review Comment:
@monkeyDluffy6017 So should I error out when the file doesn't exist or
create the file as a fallback?
##########
apisix/plugins/log-rotate.lua:
##########
@@ -312,6 +336,8 @@ end
function _M.init()
timers.register_timer("plugin#log-rotate", rotate, true)
+ custom_access_log_filename, custom_error_log_filename =
get_custom_logfile_name()
+ timers.register_timer("plugin#log-rotate", rotate, true)
Review Comment:
That was by mistake. Removed it
##########
apisix/plugins/log-rotate.lua:
##########
@@ -143,10 +157,18 @@ local function rename_file(log, date_str)
core.log.info("file exist: ", new_file)
return new_file
end
-
- 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"
+ if not exists then
+ local file = io_open(filename, "w")
+ if file then
+ file:close()
+ end
+ end
+ local ok, err = os_rename(filename, new_file)
Review Comment:
done
##########
apisix/plugins/log-rotate.lua:
##########
@@ -130,6 +132,18 @@ local function scan_log_folder(log_file_name)
return t, log_dir
end
+local function get_custom_logfile_name()
Review Comment:
done
##########
t/plugin/log-rotate.t:
##########
@@ -105,6 +106,7 @@ start xxxxxx
--- config
location /t {
content_by_lua_block {
+ local io = require("io")
Review Comment:
https://github.com/apache/apisix/pull/9619/files/232f0761a56d7589e15d77624298ddf3124737a8#diff-ec7e8ff02cbc7fd6d28666657f05dd174a7faa1e0ad33057563543db1c5618ccL121
##########
apisix/plugins/log-rotate.lua:
##########
@@ -143,10 +157,18 @@ local function rename_file(log, date_str)
core.log.info("file exist: ", new_file)
return new_file
end
-
- 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"
+ if not exists then
+ local file = io_open(filename, "w")
+ if file then
+ file:close()
+ end
+ end
Review Comment:
Is it not desirable to automatically create the log file if it does not
exist? @monkeyDluffy6017
##########
apisix/plugins/log-rotate.lua:
##########
@@ -143,10 +157,18 @@ local function rename_file(log, date_str)
core.log.info("file exist: ", new_file)
return new_file
end
-
- 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"
Review Comment:
@monkeyDluffy6017 made the change
##########
apisix/plugins/log-rotate.lua:
##########
@@ -46,8 +46,10 @@ local COMPRESSION_FILE_SUFFIX = ".tar.gz" -- compression
file suffix
local rotate_time
local default_logs
local enable_compression = false
-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"
Review 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
##########
apisix/plugins/log-rotate.lua:
##########
@@ -46,8 +46,10 @@ local COMPRESSION_FILE_SUFFIX = ".tar.gz" -- compression
file suffix
local rotate_time
local default_logs
local enable_compression = false
-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"
Review Comment:
Yes I had changed the variable name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
