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

El compatible #41

Merged
merged 8 commits into from
Feb 3, 2015
Merged

El compatible #41

merged 8 commits into from
Feb 3, 2015

Conversation

sereinity
Copy link

Make the module compatible with CentOs, RHEL, …

Benoît Marcelin added 3 commits April 4, 2014 16:25
- Split some parameters as named.conf could be in another place on some systems
- include dns::server::params when needed (puppet lint says it's a bad idea to
  inherit it)
- Align some symbols (=> or =)
- Make some requirement more readable
- Use include of params instead inherit them
- Remove useless requirements (parent dir)
@solarkennedy
Copy link
Collaborator

I would love EL support as much as the next person, but please merge from master (fixes the 400) and re-pull with RHEL specific tests.

@leonidlm
Copy link

@sereinity @solarkennedy I am wondering, why the main /etc/named.conf file isn't managed by this module?
Without adding there include "/etc/named/named.conf.local" the whole thing doesn't work on CentOS 6.x
Is it a bug / am I missing something ?

@solarkennedy
Copy link
Collaborator

Uh.. Hmmm... I don't know.

PR?

@leonidlm
Copy link

@solarkennedy Gladly, however I think you need to merge this PR so I can base mine on it. Or, I can merge this branch locally and create another PR (but then I will take @sereinity credit, which isn't good). Do you know a better way?

@sereinity
Copy link
Author

Sorry for the delay.
Yes it miss an include in main named conf. I didn't manage it because I use a lot of other setting in this file that isn't - right now - managed.

The aim of this PR is only to provide the support. I plane to add other settings in another PR, but first of all, this need to be merged.

@leonidlm
Copy link

@sereinity You are right, I am actually using your branch instead of the master (with some modifications), so no doubt that I would like to see it merged too :)

Maybe a good idea would be to use concat module to add the import to the main config file instead of managing the whole file ?

@sereinity
Copy link
Author

@leonidlm The concat module couldn't append a fragment to a file not entirely managed by concat.

But we can simply add an exec that concatenate the missing line if a grep fails. But from my point of view, it's only a workaround.

@leonidlm
Copy link

@sereinity Ohh I see, so there is no other way.
As you suggested, to manage this file directly is probably the most straight forward option!

@solarkennedy
Copy link
Collaborator

@sereinity can you rebase one more time please?

@sereinity
Copy link
Author

Yes, I will do it Thursday.

Conflicts:
	manifests/key.pp
	manifests/record.pp
	manifests/server/config.pp
	manifests/server/params.pp
	manifests/zone.pp
	spec/defines/dns__acl_spec.rb
	spec/defines/dns_zone_spec.rb
@roderickm roderickm mentioned this pull request Feb 2, 2015
@roderickm
Copy link
Contributor

@leonidlm If you only need to include a single instead of managing the entire file, perhaps the file_line source in stdlib would do what you need.

@solarkennedy solarkennedy merged commit 743a689 into ajjahn:master Feb 3, 2015
@sereinity sereinity deleted the el-compatible branch March 9, 2015 19:50
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.

4 participants