Skip to content
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

Merged
merged 2 commits into from May 9, 2015
Merged

issue 101: take control of named.conf. #112

merged 2 commits into from May 9, 2015

Conversation

ghost
Copy link

@ghost ghost commented May 7, 2015

This PR incorporates and fixes the changes proposed by chriscrowley
in PR #102:

  • control content of named.conf in addition to simple presence and ownership.
  • add to server params the additional named.conf settings that differ between Debian and RedHat
  • use a single template for both OSes
  • add tests for RedHat and update tests for Debian to ensure that named.conf is getting created with proper ownership and content.

This PR incorporates and fixes the changes proposed by chriscrowley
in PR 102.
@ghost ghost mentioned this pull request May 7, 2015
};
zone "." IN {
type hint;
file "<%= root_hint %>";
Copy link
Collaborator

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

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.

@solarkennedy
Copy link
Collaborator

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.

@ajjahn?

@ajjahn
Copy link
Owner

ajjahn commented May 8, 2015

@solarkennedy Yep, I agree. Probably should ship with a major version bump if it's going to break things. @jearls Thanks.

@ghost
Copy link
Author

ghost commented May 8, 2015

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)

@ajjahn
Copy link
Owner

ajjahn commented May 8, 2015

@jearls Right, not a breaking change unless the file is manually edited.

@solarkennedy?

@solarkennedy
Copy link
Collaborator

Right. It's just a new file that this puppet module is editing where it was not before.
I think that is fine, but other people use puppet differently.

Let me do one more code review pass.

solarkennedy added a commit that referenced this pull request May 9, 2015
@solarkennedy solarkennedy merged commit fdf7c1e into ajjahn:master May 9, 2015
@solarkennedy
Copy link
Collaborator

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!

@ghost ghost deleted the issues/101-control-named.conf branch May 9, 2015 04:24
@darkfoxprime
Copy link

@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.

@darkfoxprime darkfoxprime restored the issues/101-control-named.conf branch May 18, 2015 00:16
@darkfoxprime
Copy link

@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 /usr/share/doc/bind-9.3.6/sample/etc/

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.

@tedivm tedivm mentioned this pull request May 21, 2015
@tedivm
Copy link
Contributor

tedivm commented May 21, 2015

@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.

@darkfoxprime darkfoxprime deleted the issues/101-control-named.conf branch June 7, 2015 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants