BIND 10 #1514: Update SERIAL in DDNS

[BIND 10 Development]

#1514: Update SERIAL in DDNS
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jelte
                       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 haikuo):

 * owner:  haikuo => jelte


Comment:

 Replying to [comment:10 jelte]:
 > 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.
 sorry, forgot it. I have appended the comments for the function now.
 >
 > 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...
 OK, your codes is simpler than my codes. I will simplify this codes. For
 your concern about the split() function, I think you are right, the
 prerequisite of this function is that the soa RR must exist and not null.
 I have wrotten down it to comments of this function.
 > - 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)
 > }}}
 >
 sorry, jelte, I maybe disagree about this suggestion. :) The rfc 2136 said
 the update operation should be ignore if the serial number is smaller than
 the old one. If the new serial number is smaller than the old one, the
 serial number will be increased according your codes above.

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