You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/11/18 09:16:45 UTC
[GitHub] [apisix] monkeyDluffy6017 opened a new pull request, #8358: refactor: refactor get last index func in log rotate plugin
monkeyDluffy6017 opened a new pull request, #8358:
URL: https://github.com/apache/apisix/pull/8358
### Description
We use the `get_last_index` function to search multiple characters (https://github.com/apache/apisix/blob/3cac54c84661fcb60416afe0234dcb0115a77370/apisix/plugins/log-rotate.lua#L132), but the function is not correct now.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #8358:
URL: https://github.com/apache/apisix/pull/8358#discussion_r1142827655
##########
apisix/core/utils.lua:
##########
@@ -335,4 +337,19 @@ end
_M.resolve_var = resolve_var
+function _M.get_last_index(str, key)
+ local str_rev = str_reverse(str)
+ local key_rev = str_reverse(key)
+ local idx, _ = str_find(str_rev, key_rev)
Review Comment:
We can use plain string find here?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1473075979
> > Is this the only way?
>
> It doesn't matter if we don't change it, the log-rotate plugin is now working fine, but someone should be careful if they are going to use the `get_last_index` function later
We can put it in the core.utils so it's able to test the function?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1475864682
@spacewander done
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1321391435
can we add test cases to cover this?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #8358:
URL: https://github.com/apache/apisix/pull/8358#discussion_r1145015398
##########
apisix/core/utils.lua:
##########
@@ -335,4 +337,19 @@ end
_M.resolve_var = resolve_var
+function _M.get_last_index(str, key)
+ local str_rev = str_reverse(str)
+ local key_rev = str_reverse(key)
+ local idx, _ = str_find(str_rev, key_rev)
Review Comment:
Done
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 merged pull request #8358: refactor: use pl.stringx.rfind to replace the get_last_index func
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #8358:
URL: https://github.com/apache/apisix/pull/8358
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1473027391
> How do I write a test case for this? The original function is faulty, but the result happens to be fine @spacewander
We can provide some fake access log to see if the function can filter out the right one from the wrong ones?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by github-actions.
github-actions[bot] commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1399220977
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1321895433
> I can test the function with unit test, how can i export the local function `get_last_index`?
core.log.debug?
In fact, without testing, and with all the current test cases passing, I don't know where the original error was.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1471459666
How do I write a test case for this? The original function is faulty, but the final function happens to be fine
@spacewander
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #8358:
URL: https://github.com/apache/apisix/pull/8358#discussion_r1145564662
##########
apisix/core/utils.lua:
##########
@@ -335,4 +337,19 @@ end
_M.resolve_var = resolve_var
+function _M.get_last_index(str, key)
+ local str_rev = str_reverse(str)
+ local key_rev = str_reverse(key)
+ local idx, _ = str_find(str_rev, key_rev, 1, true)
Review Comment:
We can use core_str.find for the plain str find
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1482163188
Please make the CI pass, thanks!
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #8358:
URL: https://github.com/apache/apisix/pull/8358#discussion_r1194508541
##########
apisix/core/utils.lua:
##########
@@ -335,6 +336,21 @@ end
_M.resolve_var = resolve_var
+function _M.get_last_index(str, key)
+ local str_rev = str_reverse(str)
+ local key_rev = str_reverse(key)
+ local idx, _ = core_str.find(str_rev, key_rev)
Review Comment:
Done
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #8358:
URL: https://github.com/apache/apisix/pull/8358#discussion_r1194575889
##########
apisix/core/utils.lua:
##########
@@ -335,6 +336,21 @@ end
_M.resolve_var = resolve_var
+function _M.get_last_index(str, key)
Review Comment:
for such a basic string operation, I suggest reusing the `pl.stringx` from Penlight library. After all, we already import this lib in our code base. Never reinvent the wheel unless you have to.
https://lunarmodules.github.io/Penlight/libraries/pl.stringx.html#rfind
Example:
```
> print(require("pl.stringx").rfind("apisix__1928", "__"))
7
```
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1473029828
> get_last_index
Can I export the `get_last_index` function and do unit test?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1321422741
@tzssangglass I can test the function with unit test, how can i export the local function `get_last_index`?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #8358:
URL: https://github.com/apache/apisix/pull/8358#discussion_r1190541097
##########
apisix/core/utils.lua:
##########
@@ -335,6 +336,21 @@ end
_M.resolve_var = resolve_var
+function _M.get_last_index(str, key)
+ local str_rev = str_reverse(str)
+ local key_rev = str_reverse(key)
+ local idx, _ = core_str.find(str_rev, key_rev)
Review Comment:
`local idx = core_str.find(str_rev, key_rev)`
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #8358: refactor: use pl.stringx.rfind to replace the get_last_index func
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #8358:
URL: https://github.com/apache/apisix/pull/8358#discussion_r1194831015
##########
apisix/core/utils.lua:
##########
@@ -335,6 +336,21 @@ end
_M.resolve_var = resolve_var
+function _M.get_last_index(str, key)
Review Comment:
Done
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1473034105
> Is this the only way?
It doesn't matter if we don't change it, the log-rotate plugin is now working fine, but someone should be careful if they are going to use the `get_last_index` function later
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #8358: refactor: refactor get last index func in log rotate plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on PR #8358:
URL: https://github.com/apache/apisix/pull/8358#issuecomment-1473031837
> Can I export the `get_last_index` function and do unit test?
Is this the only way?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org