add support for importing/exporting NAPTR records #267
Conversation
|
Thanks for your contribution.
As it is, this only implements import, and cli53 actually errors on attempting to export. See failing test: Scenario: I can import a basic zone # internal/features/import.feature:7 Error: exit status 1 Output: Error: InvalidChangeBatch: [Invalid Resource Record: FATAL problem: IncompleteOctalEscapeSequence (Value contains incomplete octal escape sequence) encountered with '"" "" "/urn:cid:.+@([^\.]+.)(.*)$/\2/i"'] |
|
It works for me? |
|
Oh, I see. |
wfaulk
changed the title
|
First, despite being examples pulled directly from the NAPTR RFC, Route 53 didn't like the records I had put in the zone file. I updated them to some that it did like. I also added support for exporting NAPTR records, and it passes the test now. However, Route 53 seems to return NAPTR records in an odd/incorrect format. It always quotes the last field (replacement), which seems to be wrong, unless the last field is just (Also, go's regex support is pretty lousy. It doesn't support backreferences, which would have made this cleaner, and it claims to support non-capturing groups, but they get captured anyway.) |
There was a problem hiding this comment.
A unit test for the route53 <-> dns conversion too wouldn't hurt, here: https://github.com/barnybug/cli53/blob/master/bind_test.go#L432
Also, to come to go's defence, it doesn't sound as though you're using regexps correctly if non-capturing groups were not working for you. Rather than go itself being broken.
bind.go
Outdated
| case "NAPTR": | ||
| for _, rr := range rrset.ResourceRecords { | ||
| // parse value | ||
| naptrRE, err := regexp.Compile("^([[:digit:]]+) ([[:digit:]]+) \"([^\"]*)\" \"([^\"]*)\" \"([^\"]*)\" \"?([^\"]+)\"?$") |
There was a problem hiding this comment.
It's better to put regexs in the file scope and use MustCompile(...), so it's only compiled once.
There was a problem hiding this comment.
Also, this regexp can be written more legibly using raw strings (backticks), to cut down on the escaping required.
bind.go
Outdated
| // parse value | ||
| naptrRE, err := regexp.Compile("^([[:digit:]]+) ([[:digit:]]+) \"([^\"]*)\" \"([^\"]*)\" \"([^\"]*)\" \"?([^\"]+)\"?$") | ||
| naptr := naptrRE.FindStringSubmatch(*rr.Value) | ||
| fmt.Printf("naptr slice: %q\n", naptr) |
There was a problem hiding this comment.
Please don't leave debugging prints in the code.
|
Fixed your issues above and added a unit test NAPTR record |
Please when making a pull request:
cli53 has very good existing test coverage, so I'm unlikely to accept a pull request without
a test, but please do ping me if you're struggling to add a test case and I'll help!