-
Notifications
You must be signed in to change notification settings - Fork 17
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
support coredns v1.11.0 #76
Conversation
@chrisohaver Could you please review this PR? |
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.
The plugins map is not correct. It adds plugins that are not supported by the migration, and is missing plugins that it does support. If any new options have been added to supported plugins, those will need to be added.
I don’t know how to interpret your marked up screen shot. Please take a look at some of the prior PRs in this repo that have added a support for a new coredns version to see how they are done. Please look at the release notes for the list of plugins the migration package supports. I would not recommend adding to that list at this time. |
14b4a0b
to
ccb1474
Compare
Yes, i read prior plugins and coredns release-notes, then append plugins_1_11_0 cause's in v1.11.0 release-notes there are two new function added into plugins: so, please review my code again. Big thanks. |
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.
Thanks! Looks good! Just some minor changes requested on comments.
migration/plugins.go
Outdated
"answer name": {}, | ||
"edns0": {}, | ||
"ttl": {}, // new option | ||
"cname_target": {}, |
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.
"cname_target": {}, | |
"cname_target": {}, // new option |
migration/plugins.go
Outdated
namedOptions: map[string]option{ | ||
"apex": {}, | ||
"ttl": {}, | ||
"Fall": {}, |
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.
"Fall": {}, | |
"fallthrough": {}, // new option |
migration/plugins.go
Outdated
"name": {}, | ||
"answer name": {}, | ||
"edns0": {}, | ||
"ttl": {}, // new option |
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.
"ttl": {}, // new option | |
"ttl": {}, |
migration/versions.go
Outdated
"reload": {}, | ||
"loadbalance": {}, | ||
"hosts": plugins["hosts"]["v1"], | ||
"rewrite": plugins["rewrite"]["v3"], |
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.
"rewrite": plugins["rewrite"]["v3"], | |
"rewrite": plugins["rewrite"]["v3"], // add cname_target option |
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.
There was also a new option added to the cache plugin: keepttl
coredns/coredns#5879
Nevermind - that was added in 1.10.1 ...
... Although it looks like it's missing from the 1.10.1 migration. Eventually that should be fixed, but OK if you don't want to do it in this PR.
e1d66d1
to
7a3a352
Compare
yes,i review coredns v1.10.1 releasenotes and append cache plugin new added option keepttl. Please review it. -:) |
@chrisohaver Could you review my commit again? i hope my commit is within your expected. :) |
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.
Looks good! Thanks! Just one fix required to the name of the new fallthrough
option in k8s_external.
migration/plugins.go
Outdated
namedOptions: map[string]option{ | ||
"apex": {}, | ||
"ttl": {}, | ||
"Fall": {}, // new option |
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.
"Fall": {}, // new option | |
"fallthtrough": {}, // new option |
@@ -15,7 +15,7 @@ func TestNewValidVersionsCmd(t *testing.T) { | |||
{ | |||
name: "Works without error", | |||
expectedOutput: `The following are valid CoreDNS versions: | |||
1.1.3, 1.1.4, 1.2.0, 1.2.1, 1.2.2, 1.2.3, 1.2.4, 1.2.5, 1.2.6, 1.3.0, 1.3.1, 1.4.0, 1.5.0, 1.5.1, 1.5.2, 1.6.0, 1.6.1, 1.6.2, 1.6.3, 1.6.4, 1.6.5, 1.6.6, 1.6.7, 1.6.9, 1.7.0, 1.7.1, 1.8.0, 1.8.3, 1.8.4, 1.8.5, 1.8.6, 1.8.7, 1.9.0, 1.9.1, 1.9.2, 1.9.3, 1.9.4, 1.10.0, 1.10.1 | |||
1.1.3, 1.1.4, 1.2.0, 1.2.1, 1.2.2, 1.2.3, 1.2.4, 1.2.5, 1.2.6, 1.3.0, 1.3.1, 1.4.0, 1.5.0, 1.5.1, 1.5.2, 1.6.0, 1.6.1, 1.6.2, 1.6.3, 1.6.4, 1.6.5, 1.6.6, 1.6.7, 1.6.9, 1.7.0, 1.7.1, 1.8.0, 1.8.3, 1.8.4, 1.8.5, 1.8.6, 1.8.7, 1.9.0, 1.9.1, 1.9.2, 1.9.3, 1.9.4, 1.10.0, 1.10.1, v1.11.0 |
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.
1.1.3, 1.1.4, 1.2.0, 1.2.1, 1.2.2, 1.2.3, 1.2.4, 1.2.5, 1.2.6, 1.3.0, 1.3.1, 1.4.0, 1.5.0, 1.5.1, 1.5.2, 1.6.0, 1.6.1, 1.6.2, 1.6.3, 1.6.4, 1.6.5, 1.6.6, 1.6.7, 1.6.9, 1.7.0, 1.7.1, 1.8.0, 1.8.3, 1.8.4, 1.8.5, 1.8.6, 1.8.7, 1.9.0, 1.9.1, 1.9.2, 1.9.3, 1.9.4, 1.10.0, 1.10.1, v1.11.0 | |
1.1.3, 1.1.4, 1.2.0, 1.2.1, 1.2.2, 1.2.3, 1.2.4, 1.2.5, 1.2.6, 1.3.0, 1.3.1, 1.4.0, 1.5.0, 1.5.1, 1.5.2, 1.6.0, 1.6.1, 1.6.2, 1.6.3, 1.6.4, 1.6.5, 1.6.6, 1.6.7, 1.6.9, 1.7.0, 1.7.1, 1.8.0, 1.8.3, 1.8.4, 1.8.5, 1.8.6, 1.8.7, 1.9.0, 1.9.1, 1.9.2, 1.9.3, 1.9.4, 1.10.0, 1.10.1, 1.11.0 |
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.
... and also fix to version string in valid versions test.
correct plugins modify plugins definitions append some annotation and 1.10.1 migration update based on review comments Signed-off-by: guangli.bao <guangli.bao@daocloud.io>
e4daeb2
to
82c96aa
Compare
@chrisohaver yes, it's my fault and I corrected. I hope you are online now then review it again. Cross my finger to pray that it can be ok this time. |
Now coredns release new version: https://github.com/coredns/coredns/releases/tag/v1.11.0, so do upgrade in corefile-migration.
And some plugin changes please review if it is right in versions.go?
@chrisohaver @miekg