BIND 10 #1457: use the difference normalizer in the DDNS module

[BIND 10 Development]

#1457: use the difference normalizer in the DDNS module
-------------------------------------+-------------------------------------
                   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:  3
        Add Hours to Ticket:  0      |           Total Hours:  22
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jelte


Comment:

 Hello

 I must have been paying little attention when reading the diff, since my
 comments are a lot shorter than the diff.

 '''Log messages''':

 The text „In theory, if the datasource is implemented +correctly, no
 changes should have been made to the zone contents.“ provokes to the
 question „In theory ‒ but how about practice?“. In fact, the text is not
 very reassuring everything is OK and implies we are not confident at all
 with the data sources we ship.

 The message with id `LIBDDNS_UPDATE_DELETE_RRSET_NOT_EMPTY:` contains
 wrongly capitalized „Rrset“.

 '''Code comments''':

 {{{
 (the rrset in *wargs is replaced by the temporary one)
 }}}

 If I leave aside the fact that the function parameter is `*kwargs`, not
 `*wargs`, the sentence completely confused me. I got little bit cleaner
 after reading the code, but it should be clarified, or the function be
 more intuitive ‒ see below.

 {{{
 If this method raises an exception, these members are not set
 }}}

 This is not true. If the `find_zone` throws, there are some options set
 already. It may make sense to place the `find_zone` call first in the
 assignments.

 {{{#!python
         # (note; SOA is handled separately by __do_update, so that one
         # is not explicitely ignored here)
         if rrset.get_type() == RRType.SOA():
             return
 }}}

 The comment somewhat contradicts the code, it says it is _not_ ignored.

 {{{
 the same offort whether
 }}}
 typo: offort

 '''Code''':
 The `foreach_rr_in__rrset` is strange. It takes the result of the last
 call. Is it of any practical use? Also, I think it would be more user-
 friendly and intuitive if it returned a context, so you could write it as:
 {{{#!python
 foreach_rr(rrset):
      something_do_do(rr)
 }}}

 This should be possible, since the `TestCase.assertRaises` can be used
 this way, if the function is not passed, it returns some kind of strange
 object that makes the thing work.

 This sounds really fishy. Is such situation allowed at all?:
 {{{#!python
                     # In case there's multiple soa records in the update
                     # somehow, just take the last
                     foreach_rr_in_rrset(rrset, self.__set_soa_rrset,
 rrset)
 }}}

 In the `__do_update_delete_rrset`, what if the `find` does not return
 SUCCESS, but a DELEGATION, for example, and there's a (different) rrset
 presented for the answer?

 Considering the `__ns_deleter_helper`, what is the proper way to change
 all the NSs? I think removing them all and then adding the new ones will
 not be allowed. Also, adding first and then removing the old ones will not
 work either, as the function will take the original values, ignoring the
 new additions. Which brings me to the question ‒ how should it work if
 there are some things added and removed from the same RRset? What is the
 correct result of „add this RR“ followed by „remove whole RRset“?

 Is this a forgotten TODO, or is there a ticket to handle these?:
 {{{#!python
         if self.__added_soa is not None:
             new_soa = self.__added_soa
             # serial check goes here
         else:
             new_soa = old_soa
             # increment goes here
 }}}

 The `add_rdata` is below `create_rrset`, even when `create_rrset` uses
 `add_rdata`. I think it is not a problem syntactically, but I find it
 little bit confusing. Maybe I'm too old-fashioned and used to pascal and C
 though.

 Should the `initalize_update_rrsets` and `check_inzone_data` helper
 functions be private like so many other functions?

 Also, one more general question: What happens if RRs of the same RRset are
 interspersed with another RR? Will it create one RRset or two? Because if
 two, there'll  be problems with updates.

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