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

zone files should only be created or modified for master zones #99

Merged
merged 3 commits into from Apr 14, 2015
Merged

zone files should only be created or modified for master zones #99

merged 3 commits into from Apr 14, 2015

Conversation

ghost
Copy link

@ghost ghost commented Apr 10, 2015

The zone.pp class should not be allowed to create or modify the zone file for a slave zone at all. It should only manage master zone files.

I just added in a check to zone.pp so it only creates the concat type for master files, for all others it sets the concat to ensure => absent.

@solarkennedy
Copy link
Collaborator

Hmm, can you add some tests to cover these cases? As this manifest gets more complicated, we'll need test coverage to prevent regressions.

@ghost
Copy link
Author

ghost commented Apr 11, 2015

I can try. I have yet to actually write a test case for puppet :)

@solarkennedy
Copy link
Collaborator

I'll help you!

See the CONTRIBUTING.md for initial instructions.

Your file hint is spec/defines/dns__zone_spec.rb

It might look like something like this:

context "On a master zone" do
  let(:params) {{ :zone_type => 'master' }}
  it { should contain_concat__fragment ....
end

context "On a slave zone" do
  let(:params) {{ :zone_type => 'slave' }}
  it { should contain_concat__fragment(..).with_ensure('absent')
end

Tell me again though why on a slave zone we need this change? Certainly on a slave zone it shouldn't have an actual zone file, but it should have the configuration file fragment to declare the zone itself, right?

Johnson Earls added 2 commits April 10, 2015 18:51
…r/) for checking for allow transfer entries. fix the zone file concat checks to look for :ensure => absent or :ensure => present rather than looking for presence of the concat type itself.
@ghost
Copy link
Author

ghost commented Apr 11, 2015

actually, I think I've got what I need working (including some fixes to the existing zone spec file).

The reason the change is needed is that the bind server manages the zone file for a slave zone by itself, and that zone file should never be modified other than by the bind server.

the change I made only means that the $zone_file_stage concat is set to ensure => absent for any non-master zone; it doesn't affect creation of the zone config.

@ghost
Copy link
Author

ghost commented Apr 11, 2015

... well, i did have things working, until i merged the latest changes back in so i could stop getting the some of the test file warnings and now all the requirements have changed and i need to rebuild ruby to a newer version on my system. oh well.

@ghost
Copy link
Author

ghost commented Apr 11, 2015

Woot. Figured out how to revert the merge, ran the tests (and ignored the warning about the invalid regexp), and everything passed. Checked in the latest changes. Do I need to do anything special like try to get all the changes in one commit? (This is also my first time contributing to an open source project like this)

@solarkennedy
Copy link
Collaborator

As far as I know, this is correct.
Thank you for going all the way and writing tests to ensure that this continues to work. (I can easily see the case statement being refactored, etc.)

solarkennedy added a commit that referenced this pull request Apr 14, 2015
zone files should only be created or modified for master zones
@solarkennedy solarkennedy merged commit 17dfb7e into ajjahn:master Apr 14, 2015
@solarkennedy
Copy link
Collaborator

Thanks!

@ghost ghost deleted the only-update-master-zone-files branch April 14, 2015 18:29
@solarkennedy
Copy link
Collaborator

Strange, this seemed to pass tests in the PR but now is failing on master...

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.

1 participant