BIND 10 #1514: Update SERIAL in DDNS

[BIND 10 Development]

#1514: Update SERIAL in DDNS
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  haikuo
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:
  medium                             |  Sprint-20120612
                  Component:  DDNS   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:  DDNS   |  Estimated Difficulty:  4
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => haikuo


Comment:

 session.py:

 in the DDNS_SOA class, soa_update_check() and update_soa() have no
 docstrings, and the two other docstrings could probably use a bit of extra
 explanation about their arguments and what exactly they return.

 Also, if the method of converting to string, doing magic on it, and
 converting back is the only way to do it in python, we need better (SOA)
 rdata support in the wrappers, or maybe even in libdns++. I'm not saying
 we should do that now, but it's worth mentioning in a comment there that
 this is a very bad workaround for an incomplete API :)

 But given this code, there are a few improvements to make;
 - the for loop isn't necessary; you could just as easily do
 {{{
 soa_text_parts = origin_soa.get_rdata()[0].to_text().split()
 soa_text_parts[2] = str(soa_num.get_value())
 new_soa.add_rdata(Rdata(origin_soa.get_type(), origin_soa.get_class(),
                   " ".join(soa_text_parts)))
 }}}
 However, this does make me wonder if a basic split() even always works...
 - Not sure whether it even needs to be a class, or only a few module-level
 functions; the class does not keep any state at all (so the only purpose
 seems to be to hide the first two methods)


 I also have a few comments about the __update_soa() changes;

 - it'll fail if you actually give it a soa with a lower serial (since
 new_soa won't be set). This btw also suggest that we need to have a unit
 test that tries that :)
 - the assignment of old to new isn't really necessary in the else-
 statement; doing the update_soa directly on old_soa should also work
 - both of the above can i think be fixed by merging the nested ifs;
 something like
 {{{
 if added_soa is not None and serial_update_check:
   new_soa = added_soa
 else:
   new_soa = update_serial(old_soa)

 (names not reflecting actual current code)
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/1514#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development