Update zone-serial only on changing zone-records (sed version) #45

kubashin-a · 2014-04-09T17:45:09Z

Sed version of PR #42.

I tested it only on Ubuntu 14.04 (Puppet 3.4.3).

kubashin-a · 2014-04-09T17:50:45Z

manifests/zone.pp

+    # A real zone file will be updated only at change of the stage file, thanks to this serial is updated only in case of need
+    $zone_serial = inline_template('<%= Time.now.to_i %>')
+    exec { "real-${zone}":
+      command     => "sed '8s/_SERIAL_/${zone_serial}/' ${zone_file_stage} > ${zone_file}",


I change only line num 8 (sed '8s/...'). It is safe, but demands care with a zone_file.erb template.

That seems ok to me.

solarkennedy · 2014-04-10T04:24:13Z

I think we are getting close to a solid PR.

The next thing I would like to talk about it is tests. Have you ever written a Puppet rspec test before?

solarkennedy · 2014-04-10T05:06:25Z

The first most basic test is lint.

Now, this module hasn't had lint tests against it historically, so there are lots of previous errors.

But can you fix the ones that pertain to the file you have edited? (zone.pp)

Start by installing the necissary gems to run the test:

bundle install

Then run the lint test, and only look at the errors that are related to your change:

bundle exec rake lint | grep zone.pp

(In the root of the module directory. Requires ruby and rubygems to be installed)

kubashin-a · 2014-04-10T06:43:05Z

I never wrote tests for Puppet. I can try this, but it will take time.

kubashin-a · 2014-04-10T19:05:42Z

manifests/zone.pp

+    # A real zone file will be updated only at change of the stage file, thanks to this serial is updated only in case of need
+    $zone_serial = inline_template('<%= Time.now.to_i %>')
+    exec { "bump-${zone}-serial":
+      command     => "sed '8s/_SERIAL_/${zone_serial}/' ${zone_file_stage} > ${zone_file}",


I checked code by lint and corrected all warnings except this line (correct version not committed now). Warning for this line:

manifests/zone.pp:56:80chars:WARNING:line has more than 80 characters

There is a need to correct this warning?

Eh, its ok.

solarkennedy · 2014-04-11T02:55:49Z

Linting is good. Feel free to run bundle exec rake lint whenever, to make sure you are as close to the style guide as reasonable.

Next is rspec tests. They are currently failing.

https://travis-ci.org/ajjahn/puppet-dns/jobs/22713442

Run the tests yourselves on your own code:

bundle install #you've already run this

bundle exec rake spec

You will see test cases that are defined in the spec folder. Check them out, see why some are failing, etc. Try to get it to come out green.

kubashin-a · 2014-04-13T20:09:40Z

My local puppet was broken (after "bundle install") therefore, I couldn't check operability of the last patch. I will try verify code tomorrow, but i think that everything is good.

solarkennedy · 2014-04-13T20:54:03Z

Hmm, depending on whether you installed puppet via a Gem or system package, there might be a conflict. The puppet "binary" should always prefer the Gem install.

If you wanted to you could uninstall the gem version, set a local GEM_HOME=~/.gem/ and install gems locally to prevent them from conflicting.

solarkennedy · 2014-04-13T21:02:10Z

Getting the existing tests to pass is a good first step, but we need tests on the new code in order to be accepted. Here is another PR with some tests in progress for some examples: https://github.com/ajjahn/puppet-dns/pull/46/files

There are also existing tests to look at.

For your purposes, you will be working with spec/defines/dns_zone_spec.rb, which contains tests for the functionality of the zone logic.

For this, PR I would like to see tests for:

The zone file has the right filename (it is now called "staging")
That the zone file fragment has the word SERIAL in it, otherwise it won't get caught by sed
That the right exec is in the catalog, with refreshonly => true

You can't test that the actual zone file serial gets bumped for real in unit tests, however :(

But testing that those 3 things exist in the catalog is good enough for me, and assures me that if something changes in the future that breaks the functionality you just wrote, we will know because the tests will fail!

danzilio · 2014-04-14T04:27:13Z

Sorry I've been largely absent from this discussion, and maybe this is beyond the scope, but do you guys think this functionality could be better accomplished with a type/provider? Just a thought.

solarkennedy · 2014-04-14T04:35:19Z

Hmmm. Interesting idea.

On the one hand, yes, a bind provider could handle the updating of the serial number "on flush".

But in the end, the bind configuration syntax is not standard, and certainly not something machine readable like json. The provider would have a really hard time extracting the current state of the configuration files. You basically would end up writing a zone file parser in ruby.

It would be cool, but would put this module outside the scope of most puppet module writers skillsets (including mine). :(

Erb+concat really does get us most of the way there, and it can be understood by most people (sysadmins). The only special thing about zone files is that serial bump, so I think using sed is a good middle-ground-hack. (Too bad bind couldn't just handle bumping serials internally automatically some how)

solarkennedy · 2014-04-16T14:54:36Z

@kubashin-a can you merge master (has changed recently) ?

kubashin-a · 2014-04-17T17:43:11Z

I merged maser.
Sorry, I don't have time for work on this feature now, but i'll try to make tests in this weekend.

solarkennedy · 2014-04-18T14:38:35Z

Cool. I look forward to this being merged. If not by you then I will pick it up and complete it.

kubashin-a · 2014-04-19T17:49:15Z

spec/defines/dns_zone_spec.rb

@@ -4,7 +4,7 @@
  let(:title) { 'test.com' }

  context 'passing something other than an array' do
-    let :facts  do { :concat_basedir => '/dne',  } end
+    let :facts  do { :osfamily => 'Debian', :concat_basedir => '/dne' } end


include dns::server::params in zone.pp require fact osfamily

That is fine.

kubashin-a · 2014-04-20T07:16:53Z

spec/defines/dns_zone_spec.rb

+    }
+
+    it { should contain_concat__fragment('db.test.com.soa').
+      with_content(/_SERIAL_/)


There is a method to check only the 8th line for pattern?

Not that I know of, but I'm pretty happy with this part.

If someone breaks the method of bumping serials, this test will complain.

solarkennedy · 2014-04-20T15:46:17Z

There are still merge conflicts on master. Can you merge upstream master and correct them so, and then travis should run?

git remote add upstream git@github.com:ajjahn/puppet-dns.git
git fetch upstream master
git merge upstream/master
# (optionally rebase if you know what you are doing)

kubashin-a · 2014-04-20T15:58:30Z

I executed these commands, now:

$ git fetch upstream master
From https://github.com/ajjahn/puppet-dns
 * branch            master     -> FETCH_HEAD
$ git merge upstream/master
Already up-to-date.
$ git push origin master
Everything up-to-date

solarkennedy · 2014-04-20T15:59:35Z

Hmm. And were you on your sed branch at the time of the merge?

solarkennedy · 2014-04-20T16:01:16Z

Here is I get when I try to merge your branch master from your branch: https://pastee.org/2qcyg

kubashin-a · 2014-04-20T16:04:10Z

Yes, i have the same picture. I go to read about the merge conflicts :)

Conflicts: manifests/zone.pp spec/defines/dns_zone_spec.rb

kubashin-a · 2014-04-20T16:35:44Z

In the current master an error in zone.pp used variable: $dns::options::forwarder, but whould be $dns::server::options::forwarder.

solarkennedy · 2014-04-20T16:36:38Z

You should be able to correct it, commit it, and push again.

kubashin-a · 2014-04-20T17:18:18Z

It work on Ubuntu 14.04, do you need additional testing?

solarkennedy · 2014-04-20T18:42:04Z

This looks good to me. I'm was mostly concerned about unit tests. (and Travis now sees them as passing)
I (personally) am satisfied with the implementation, and the testing, and the little things.

@danzilio or @ajjahn do you have any objections? I vote merge. This solves the non-convergence issue that we've had since the beginning. (and I don't like non-converging puppet runs)

Update zone-serial only on changing zone-records (sed version)

solarkennedy · 2014-04-22T04:56:10Z

@kubashin-a Thank you again for iterating over this PR for so long to refine it. In the end I think it is pretty solid.

kubashin-a · 2014-04-22T06:38:20Z

Big thanks for your work and patience.

Update zone-serial only on changing zone-records

47fc592

kubashin-a reviewed Apr 9, 2014
View reviewed changes

Cleanup code of zone-serial feature

b2dcdb4

kubashin-a reviewed Apr 10, 2014
View reviewed changes

Errors found through tests are corrected

d3f8af7

This was referenced Apr 14, 2014

Update zone-serial only on changing zone-records #42

Closed

Change zone-serial only on record updates (this a solution) #24

Closed

adding tests for dns::zone

1c4a7f0

kubashin-a reviewed Apr 19, 2014
View reviewed changes

fix test

c38b234

kubashin-a reviewed Apr 20, 2014
View reviewed changes

kubashin-a added 2 commits April 20, 2014 20:19

Merge remote-tracking branch 'upstream/master' into zone-serial-sed

81fc115

Conflicts: manifests/zone.pp spec/defines/dns_zone_spec.rb

fix test after master merge

e3817f1

fix variable ::server::options::forwarder in zone.pp

f8b0b15

solarkennedy added a commit that referenced this pull request Apr 22, 2014

Merge pull request #45 from kubashin-a/zone-serial-sed

ba00d09

Update zone-serial only on changing zone-records (sed version)

solarkennedy merged commit ba00d09 into ajjahn:master Apr 22, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update zone-serial only on changing zone-records (sed version) #45

Update zone-serial only on changing zone-records (sed version) #45

kubashin-a commented Apr 9, 2014

kubashin-a Apr 9, 2014

solarkennedy Apr 10, 2014

solarkennedy commented Apr 10, 2014

solarkennedy commented Apr 10, 2014

kubashin-a commented Apr 10, 2014

kubashin-a Apr 10, 2014

solarkennedy Apr 11, 2014

solarkennedy commented Apr 11, 2014

kubashin-a commented Apr 13, 2014

solarkennedy commented Apr 13, 2014

solarkennedy commented Apr 13, 2014

danzilio commented Apr 14, 2014

solarkennedy commented Apr 14, 2014

solarkennedy commented Apr 16, 2014

kubashin-a commented Apr 17, 2014

solarkennedy commented Apr 18, 2014

kubashin-a Apr 19, 2014

solarkennedy Apr 19, 2014

kubashin-a Apr 20, 2014

solarkennedy Apr 20, 2014

solarkennedy commented Apr 20, 2014

kubashin-a commented Apr 20, 2014

solarkennedy commented Apr 20, 2014

solarkennedy commented Apr 20, 2014

kubashin-a commented Apr 20, 2014

kubashin-a commented Apr 20, 2014

solarkennedy commented Apr 20, 2014

kubashin-a commented Apr 20, 2014

solarkennedy commented Apr 20, 2014

solarkennedy commented Apr 22, 2014

kubashin-a commented Apr 22, 2014

Update zone-serial only on changing zone-records (sed version) #45

Update zone-serial only on changing zone-records (sed version) #45

Conversation

kubashin-a commented Apr 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solarkennedy commented Apr 10, 2014

solarkennedy commented Apr 10, 2014

kubashin-a commented Apr 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solarkennedy commented Apr 11, 2014

kubashin-a commented Apr 13, 2014

solarkennedy commented Apr 13, 2014

solarkennedy commented Apr 13, 2014

danzilio commented Apr 14, 2014

solarkennedy commented Apr 14, 2014

solarkennedy commented Apr 16, 2014

kubashin-a commented Apr 17, 2014

solarkennedy commented Apr 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solarkennedy commented Apr 20, 2014

kubashin-a commented Apr 20, 2014

solarkennedy commented Apr 20, 2014

solarkennedy commented Apr 20, 2014

kubashin-a commented Apr 20, 2014

kubashin-a commented Apr 20, 2014

solarkennedy commented Apr 20, 2014

kubashin-a commented Apr 20, 2014

solarkennedy commented Apr 20, 2014

solarkennedy commented Apr 22, 2014

kubashin-a commented Apr 22, 2014