Trying again at using the python samba_kcc

[garming at catalyst.net.nz]

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
- 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?
- 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?

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.

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

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
>
>
>