Add zone metadata resource#83
Add zone metadata resource#83selfuryon wants to merge 10 commits intopan-net:masterfrom selfuryon:add_meta
Conversation
| ResourceName: resourceName, | ||
| ImportState: true, | ||
| ImportStateVerify: true, | ||
| ImportStateVerifyIgnore: []string{"nameservers"}, |
There was a problem hiding this comment.
Current tests in master are broken, so I ignore nameservers option in these tests
There was a problem hiding this comment.
Its got back life. You can pull reverted commit and remove this part
powerdns/client.go
Outdated
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| if resp.StatusCode != 200 { |
There was a problem hiding this comment.
Would suggest to change it back here. Also at some other places(I will point). Let keep the same style. http.StatusOK looks more clear at least for me.
There was a problem hiding this comment.
oh, I got it, It was unclear for me because both styles are exists in the code.
There was a problem hiding this comment.
Well, I am about to refactor the project a bit in near feature. 🙂
powerdns/client.go
Outdated
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != 200 { |
using constants instead of numbers
|
I will do the review more thoroughly either during the weekend or on monday. So far it looks nice. |
|
|
||
| for _, mt := range mtdata { | ||
| if len(strings.Trim(mt.(string), " ")) == 0 { | ||
| log.Printf("[WARN] One or more values in 'metadata' contain empty '' value(s)") |
There was a problem hiding this comment.
I assume here was something in single quotes but you deleted it but quotes are left.
Plus you don't break this loop if you find empty metadata.
In case we will have N objects in list empty - it will through the message N times.
I would suggest add a break after this log.
There was a problem hiding this comment.
And also there is a function TrimSpace in strings package. We can use it instead of regular trim :)
|
The rest is fine. Can be merged when discussion above closed. |
|
Is there anything blocking this PR from being merged? |
|
@pbnsh I guess nothing. It just slipped my mind. Sorry. I will review it once again just so that , just to make sure everything is OK, and merge it ASAP. |
4 participants
Add additional resource for managing zone metadata based upon #60