-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pathrouting fix #1190
Pathrouting fix #1190
Conversation
Would be good to update the unit tests. For example here we are passing an empty table. There are a few instances of this in the unit test, would be nice to add a test for both the happy and unhappy paths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Kevin mentioned, a new unit test would be super helpful here.
Regards
@@ -17,7 +17,7 @@ function _M.find_service(config_store, host) | |||
if hosts[h] == host then | |||
local name = service.system_name or service.id | |||
ngx.log(ngx.DEBUG, 'service ', name, ' matched host ', hosts[h]) | |||
local matches = mapping_rules_matcher.matches(method, uri, {}, service.rules) | |||
local matches = mapping_rules_matcher.matches(method, uri, ngx.req.get_uri_args(), service.rules) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok, but I would move this to the top. Calling to a function is expensive, and we should be worried about.
Do the same as method, uri, etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sure.
This PR fix THREESCALE-5149