-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fallback ko #2
Fallback ko #2
Conversation
@johnbelamaric , I'm afraid you need to rebase your branch. Obviously, my changes are only in the fallback plugin. All the other changes are recent merges. |
And, I still have to update the README. |
d552f71
to
35dc8be
Compare
Ok, I rebased my branch. I guess you now need to rebase your PR on my new branch? |
Wrapping up the README. I'll give that a shot when that's done. |
@johnbelamaric , I have updated the README, and have also resolved the conflicts. It's now ready for your review. Thanks!! |
plugin/fallback/README.md
Outdated
This plugin will send queries to an alternate set of upstreams if the plugin | ||
## Name | ||
|
||
*fallback* - a plugin that send queries to an alternate set of upstreams if the plugin |
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.
s/send/sends/
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.
Will fix
plugin/fallback/README.md
Outdated
chain returns specific error messages. | ||
|
||
## Description | ||
|
||
The *fallback* plugin allows an alternate set of upstreams be specified and which will be used |
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.
s/and which/which/
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.
Will fix.
plugin/fallback/upstream_utils.go
Outdated
) | ||
|
||
// upstreamMapper manages map of rcode to upstream | ||
type upstreamMapper interface { |
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.
I don't understand why we need this separate interface and struct. It doesn't really do anything more than the map did already. Just seems like extra stuff.
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.
Yeah.... I was going to use that to test setup().
OK, I'll revert it back to the original rules map.
plugin/fallback/upstream_utils.go
Outdated
type fallbackProxyCreator struct{} | ||
|
||
func (f fallbackProxyCreator) Create(trace plugin.Handler, upstream proxy.Upstream) plugin.Handler { | ||
return &proxy.Proxy{Trace: trace, Upstreams: &[]proxy.Upstream{upstream}} |
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.
Same here - simpler to just have this one line of code in the setup?
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.
Please let me clarify. The proxyCreator
interface is introduced so that I can write test to verify whether proxy is created using the expected upstream. This is the implementation that is actually used in the fallback plugin. There is a fake one used by unit test.
Not sure if this answer you question...
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.
Yeah, I figured it was for unit tests but somehow I missed that it was in fact used. OK.
Remove upstreamMapper code. Other minor fixes.
@johnbelamaric , I have addressed all the comments. PTAL. Thanks!! |
1. What does this pull request do?
Add unit tests to the fallback plugin PR coredns#1420
2. Which issues (if any) are related?
coredns#1382
3. Which documentation changes (if any) need to be made?
README.md yet to be updated.