You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Revolyssup (via GitHub)" <gi...@apache.org> on 2023/05/24 06:07:51 UTC
[GitHub] [apisix] Revolyssup opened a new pull request, #9538: fix: replace mock tests with real tests in http logger
Revolyssup opened a new pull request, #9538:
URL: https://github.com/apache/apisix/pull/9538
### Description
Replaces existing mock tests in http logger with real tests using vector as http log server
### 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
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] 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)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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] Revolyssup commented on a diff in pull request #9538: fix: replace mock tests with real tests in http logger
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9538:
URL: https://github.com/apache/apisix/pull/9538#discussion_r1208868536
##########
t/plugin/udp-logger.t:
##########
@@ -425,7 +425,6 @@ qr/.*plugin_metadata.*/
end
ngx.say(body)
-
Review Comment:
Yes but I am unsure. I was getting lint error previously so I added this new line.
--
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] Revolyssup commented on a diff in pull request #9538: fix: replace mock tests with real tests in http logger
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9538:
URL: https://github.com/apache/apisix/pull/9538#discussion_r1208892117
##########
t/plugin/http-logger-log-format.t:
##########
@@ -438,7 +439,7 @@ done
[[{
"log_format": {
"host": "$host",
- "labels": "$a6_route_labels",
+ "labels": "custom variable check",
Review Comment:
Okay this ctx variable was set here https://github.com/apache/apisix/blob/73ac77d22b4ae0c2b597105e778f9a5d6ea86e3d/t/plugin/http-logger-log-format.t#L491
Added it back
--
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] Revolyssup commented on a diff in pull request #9538: fix: replace mock tests with real tests in http logger
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9538:
URL: https://github.com/apache/apisix/pull/9538#discussion_r1208879143
##########
t/plugin/http-logger-log-format.t:
##########
@@ -438,7 +439,7 @@ done
[[{
"log_format": {
"host": "$host",
- "labels": "$a6_route_labels",
+ "labels": "custom variable check",
Review Comment:
Actually the labels passed in route were not being reflected on the log so I had removed $a6_route_labels. Checking this right now...
--
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 #9538: fix: replace mock tests with real tests in http logger
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9538:
URL: https://github.com/apache/apisix/pull/9538#issuecomment-1573034294
@Revolyssup please resolve the conflicts
--
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 #9538: fix: replace mock tests with real tests in http logger
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9538:
URL: https://github.com/apache/apisix/pull/9538#issuecomment-1563040138
@Sn0rt please help to review
--
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] Revolyssup commented on a diff in pull request #9538: fix: replace mock tests with real tests in http logger
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on code in PR #9538:
URL: https://github.com/apache/apisix/pull/9538#discussion_r1208868536
##########
t/plugin/udp-logger.t:
##########
@@ -425,7 +425,6 @@ qr/.*plugin_metadata.*/
end
ngx.say(body)
-
Review Comment:
Yes but I am unsure. I was getting lint error previously so I added this new line. @monkeyDluffy6017
--
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] Revolyssup commented on pull request #9538: fix: replace mock tests with real tests in http logger
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9538:
URL: https://github.com/apache/apisix/pull/9538#issuecomment-1573124160
@monkeyDluffy6017 Please approve CI
--
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] Revolyssup commented on pull request #9538: fix: replace mock tests with real tests in http logger
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #9538:
URL: https://github.com/apache/apisix/pull/9538#issuecomment-1573094747
> @Revolyssup please resolve the conflicts
@monkeyDluffy6017 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 a diff in pull request #9538: fix: replace mock tests with real tests in http logger
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9538:
URL: https://github.com/apache/apisix/pull/9538#discussion_r1208852379
##########
t/plugin/http-logger-log-format.t:
##########
@@ -438,7 +439,7 @@ done
[[{
"log_format": {
"host": "$host",
- "labels": "$a6_route_labels",
+ "labels": "custom variable check",
Review Comment:
Why do you change this? The title is `use custom variable in the logger`, but you don't check the variable 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] monkeyDluffy6017 commented on pull request #9538: fix: replace mock tests with real tests in http logger
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9538:
URL: https://github.com/apache/apisix/pull/9538#issuecomment-1569381125
Could you merge the master? The ci always failed because the go packages faile to download.
--
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 #9538: fix: replace mock tests with real tests in http logger
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #9538:
URL: https://github.com/apache/apisix/pull/9538
--
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 #9538: fix: replace mock tests with real tests in http logger
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9538:
URL: https://github.com/apache/apisix/pull/9538#discussion_r1208856638
##########
t/plugin/udp-logger.t:
##########
@@ -425,7 +425,6 @@ qr/.*plugin_metadata.*/
end
ngx.say(body)
-
Review Comment:
The `ngx.say` has nothing to do with the code below, so the blank is needed?
--
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 #9538: fix: replace mock tests with real tests in http logger
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9538:
URL: https://github.com/apache/apisix/pull/9538#discussion_r1208852379
##########
t/plugin/http-logger-log-format.t:
##########
@@ -438,7 +439,7 @@ done
[[{
"log_format": {
"host": "$host",
- "labels": "$a6_route_labels",
+ "labels": "custom variable check",
Review Comment:
Why you change this?
##########
t/plugin/http-logger-log-format.t:
##########
@@ -438,7 +439,7 @@ done
[[{
"log_format": {
"host": "$host",
- "labels": "$a6_route_labels",
+ "labels": "custom variable check",
Review Comment:
Why do you change 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