feat: Support defining records by dns zone format#1360
feat: Support defining records by dns zone format#1360kwitsch merged 22 commits into0xERR0R:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1360 +/- ##
==========================================
+ Coverage 93.96% 93.97% +0.01%
==========================================
Files 78 78
Lines 6343 6361 +18
==========================================
+ Hits 5960 5978 +18
Misses 294 294
Partials 89 89 ☔ View full report in Codecov by Sentry. |
|
My thoughts on the open questions:
I opt for removal since it's not released yet.
There is a struct in helpertest to generate temporary folders and subsequently string based files for testing. If you look for the usages it should be obvious how to use it in tests. 😉
It should be enough to list what is currently not/supported and a link to an proper zonefile documentation (documenting it ourselves is a bit out of scope). 🤔
customDNS.customTTL should only apply to the old format (documentation should mention it) in my opinion. |
docs/configuration.md
Outdated
| | customTTL | duration (no unit is minutes) | no | 1h | | ||
| | rewrite | string: string (domain: domain) | no | | | ||
| | mapping | string: string (hostname: address or CNAME) | no | | | ||
| | zoneFileMapping | multiline string containing a DNS Zone File | no | | |
There was a problem hiding this comment.
Since it supports already more than the simple mapping and is no file by itself I would opt for zone as config key. 🤔
|
Perfect! Thank you for your input @kwitsch. I'll implement these changes as I have some free time in the next day or 2 |
ThinkChaos
left a comment
ThinkChaos
left a comment
There was a problem hiding this comment.
Thanks for tackling this!
I agree with everything @kwitsch said above :)
Just have very minor comments.
config/custom_dns.go
Outdated
| return err | ||
| } | ||
|
|
||
| result := make(ZoneFileDNS, len(input)) |
There was a problem hiding this comment.
Minor copy-paste typo (1 seems sensible since you're probably at least going to have a domain if you specify the option):
| result := make(ZoneFileDNS, len(input)) | |
| result := make(ZoneFileDNS, 1) |
config/custom_dns_test.go
Outdated
| }) | ||
|
|
||
| Describe("UnmarshalYAML", func() { | ||
| Describe("#CustomDNSEntries UnmarshalYAML", func() { |
There was a problem hiding this comment.
Does # do anything here?
There was a problem hiding this comment.
Nope! I will remove it when I work on this next
|
This should be ready for another round of reviews 😄 |
|
Gonna look into it tomorrow. 🙂👍 |
ThinkChaos
left a comment
ThinkChaos
left a comment
There was a problem hiding this comment.
Minor stuff, except the $INCLUDE thing which is a bit bigger but hopefully has enough guidance to follow!
…as relative filenames when using the $INCLUDE directive
ThinkChaos
left a comment
ThinkChaos
left a comment
There was a problem hiding this comment.
Two details, besides that everything looks good to me!
…se when config param passed to blocky is a directory
ThinkChaos
left a comment
ThinkChaos
left a comment
There was a problem hiding this comment.
Thanks again, this is great!
|
@kwitsch I didn't merge in case you had somethign to add. Anything left? If not can you press the green button? :) |
✅ |
3 participants
Closes #1355
As I got to looking at the issue above, it became obvious to me that support for DNS zone files only requires a very small change to my previous PR.
Outstanding questions for others:
CNAME()method undermappings? This hasn't been published as far as I can tell, so we have the option to remove it before it hits the mainline if we don't like/want that syntax moving forward for non-A records.$INCLUDEdirective?$GENERATEdirective and expect others to use it?