Trying again at using the python samba_kcc

[Douglas Bagnall]

hi Garming,

Thanks. I've addressed these things and pushed to 
http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/heads/kcc-intersite-27

On 15/05/15 22:27, garming at catalyst.net.nz wrote:
> Hi,
> 
> So I've spent some time reviewing some of the patches. The last dozen or
> so I've left off for now since Douglas hasn't signed off on them yet.
> 
> I've looked over the changes and they all seem fairly reasonable. I've
> made my notes in the form of TODO on the tree.
> 
> Douglas:
> - The edge list always being empty in your comment is slightly worrying

Yes, but fortunately I was wrong -- or at least I have since patched
up and rebased away the thing that made me right.

> - There's a few commits that should probably be merged without too much
> trouble

> - There is a commit that implies RO is partial when it may not be the case?

That was not about RO at all, but I can see how the comment didn't
explain that well enough. If the patch had 60 lines of context it
would be clear. I hope this message improves it:

http://git.catalyst.net.nz/gitweb?p=samba.git;a=commitdiff;h=1566d43ae

(vs the original, with Garming's query:
https://git.samba.org/?p=garming/samba.git;a=commit;h=8e180eb7ec51 )

> - There's some code you highlighted that might be dealing with schedules
> wrong - should it be fixed? And in general, what should None schedules
> mean? I think you've noted that there are differences with it being always
> in some places and being that 15 minute block every hour in others - could
> it always mean the latter?

I have clarified the comment you highlighted. 

The source of the confusion is that there are two schedule types
(ReplTimes and Schedule) used by different objects, each packaging the
same information in different ways. In the specification, both types
are called "schedule" in variables and attributes, and we followed the
spec in our naming. In the original C++ these would have been
disambiguated by the declared types, but in python it is unclear which
schedule type is in use at any point. A null Schedule tends to mean
"never" while a null ReplInfo tends to mean "always".

> The last things I think is that, I see you've removed the SMTP code among
> other things. But you've also removed some of the failed link and site
> link bridge code. For the stuff that could be implemented still and isn't
> stubbed out anymore, it should probably be noted well somewhere. I still
> think there might be some cases which are executed in the remove
> unnecessary failed links but we sort of would need a local store before it
> made more sense.

Right. I've reverted that one. I think this part:

-        # Remove all tuples in kcc_failed_links where failure count = 0
-        # In this implementation, this should never happen.

really doesn't apply to us, but I haven't thought through the other bit.

> I've pushed it with my review to my public Samba git repo:
> 
> git://git.samba.org/garming/samba.git
> https://git.samba.org/?p=garming/samba.git;a=shortlog;h=refs/heads/kcc-intersite-to-review

There is one TODO commit by abartlet at samba.org. Andrew, can you look
at it?

http://git.catalyst.net.nz/gitweb?p=samba.git;a=commitdiff;h=ab587cc2f4ff

cheers,

Douglas


> Cheers,
> 
> Garming
> 
>> On Wed, 2015-05-13 at 17:28 +1200, Douglas Bagnall wrote:
>>> hi all,
>>>
>>> On 18/04/15 01:46, Jelmer Vernooij wrote:
>>>> On Fri, Apr 17, 2015 at 09:57:23PM +1200, Andrew Bartlett wrote:
>>>>> On Fri, 2015-04-17 at 21:44 +1200, Douglas Bagnall wrote:
>>>>>> On 17/04/15 01:06, Jelmer Vernooij wrote:
>>> [...]
>>>>>>> * samba.ldif_utils seems misnamed - it seems like it's more
>>> samba.samdb.ldif_utils?
>>>>>>
>>>>>> Right. I'll look at that next week.
>>>>>
>>>>> I'm thinking that the KCC object belongs in samba.kcc, then we have
>>>>> samba.kcc.utils and samba.kcc.ldif_import_export (which is what the
>>>>> ldif_utils is for, it looks generic but really isn't).
>>>>
>>>> Yes. That would also make unit testing the various parts of it much
>>>> easier.
>>>
>>> We have restructured the code in this way, with a smallish samba_kcc
>>> script and modules in samba.kcc.*. There are unit tests for all the
>>> modules, though they are far from comprehensive.
>>>
>>> One unfortunate affect of this rearrangement is that `git blame` and
>>> friends get confused and tend not to look back beyond the move.
>>> Another is that it makes it hard to address one of Andreas's
>>> complaints -- that of documentation strings being introduced in a
>>> later commit than the function that they document. It is difficult to
>>> push the docstring patches back across the rename barrier.
>>>
>>> http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/heads/kcc-intersite-26
>>
>> Thanks Douglas,
>>
>> I'll add my review marker to this shortly, and tidy up a stray DEBUG(0)
>> in the timeout patch early in the series.   Then I'll propose just the
>> first few tidy-up patches.  The whole series is very large, but we need
>> to make a start on getting them into master.
>>
>> Garming,
>>
>> You know this code best, can you please add your review where
>> appropriate?
>>
>> Thanks,
>>
>> --
>> Andrew Bartlett                       http://samba.org/~abartlet/
>> Authentication Developer, Samba Team  http://samba.org
>> Samba Developer, Catalyst IT
>> http://catalyst.net.nz/services/samba
>>
>>
>>
> 
>