-
Notifications
You must be signed in to change notification settings - Fork 112
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
issue 101: take control of named.conf. #112
Conversation
This PR incorporates and fixes the changes proposed by chriscrowley in PR 102.
}; | ||
zone "." IN { | ||
type hint; | ||
file "<%= root_hint %>"; |
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.
use @root_hint
for puppet 4 compat
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.
Whoops, I missed that one, sorry! fix committed.
Thank you for doing this! I think long term it makes sense for puppet to manage this. I think that it is a natural progression for this module. This does lead to a potentially breaking change that I would like the other developer to weigh in on. |
@solarkennedy Yep, I agree. Probably should ship with a major version bump if it's going to break things. @jearls Thanks. |
Is this actually a breaking change? Unless people modified the default named.conf by hand, this replaces the Debian named.conf with equivalent functionality (i think, I don't have a debian box to test on) and makes it actually work on RedHat (where the default named.conf never sees the changes that this module is making) |
@jearls Right, not a breaking change unless the file is manually edited. |
Right. It's just a new file that this puppet module is editing where it was not before. Let me do one more code review pass. |
issue 101: take control of named.conf.
Great! I'll bump the major version so the next time we do a release it will be on a new, major version. Thanks again @jearls for taking a PR and following up with it! |
@solarkennedy go ahead and revert this change. I thought I had tested it properly, but I guess not. I'll re-create the changes and submit a new PR at a later time, once I'm sure it really works. |
@solarkennedy - to make this work properly in RedHat, I think we're going to need to take over all the default files that named technically provides. In 6.x, those files are installed under /etc, but in 5.x, they're in By taking over control of all those files, or just incorporating them into the default named.conf, we should be able to work with both Debian and RedHat because we won't be dependent on the files that are installed (or not) by the package. |
@darkfoxprime / @jearls - The issues weren't too hard to resolve, and I took care of them in #119. I'd suggest just merging that back in and forking off the master for corrections. If the issue is that bind is giving different defaults to RedHat than Debian I'd suggest not trying to take over the /usr/share/doc files (as they can get overwritten during updates) but just putting the required changes into the named.conf file only when the system is RedHat. In other words, issue a quick conditional in the template and if it's redhat put the root zone and logging code into named.conf and otherwise leave it along. |
This PR incorporates and fixes the changes proposed by chriscrowley
in PR #102: