[Freeipa-devel] [PATCHES 0066-0067] Upgrade procedure for forwardzones

[Martin Kosek]

On 06/25/2014 05:07 PM, Martin Basti wrote:
> On Wed, 2014-06-25 at 14:36 +0200, Martin Kosek wrote:
>> On 06/24/2014 04:52 PM, Martin Basti wrote:
>>> On Tue, 2014-06-24 at 16:36 +0200, Martin Kosek wrote:
>>>> On 06/18/2014 01:46 PM, Martin Basti wrote:
>>>>> On Wed, 2014-06-18 at 13:44 +0200, Martin Basti wrote:
>>>>>> On Fri, 2014-06-13 at 10:28 +0200, Martin Basti wrote:
>>>>>>> Patches attached, require patches mbasti 0052-0055.
>>>>>>> _______________________________________________
>>>>>>> Freeipa-devel mailing list
>>>>>>> Freeipa-devel at redhat.com
>>>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>>> Rebased patches attached.
>>>>>> PEP8 fixes.
>>>>>>
>>>>> Sorry, patches are here
>>>>
>>>> 66.2:
>>>>
>>>> 1) Is it OK to just change constants in the update plugins?
>>>>
>>>> -PRE_UPDATE = 1
>>>> -POST_UPDATE = 2
>>>> +PRE_SCHEMA_UPDATE = 1
>>>> +PRE_UPDATE = 2
>>>> +POST_UPDATE = 3
>>>>
>>>> When people refer to the types via names, it should be OK. It just seemed
>>>> unnecessary to me.
>>>>
>>> I checked where the constants are used, and it shouldn't broke anything.
>>> It looks weird to me to have something which happens first with last
>>> number.
>>> Should I set PRE_SCHEMA_UPDATE = 3 and leave other unchanged then?
>>
>> Ok, either update it or set it to 0. Up to you.
>>
>>>
>>>> 67.2:
>>>>
>>>> 1) update_check_forwardzones:
>>>>
>>>> I think we should set update_to_forward_zones to False when the objectclass is
>>>> there and add a check at the beginning of the execute to simply bail out, if
>>>> update_to_forward_zones is present in the sysupgrade file.
>>>>
>>>> This will prevent the objectclass check (which takes some time) to run again
>>>> and again.
>>> Good point thanks.
>>>
>>>> 2) I would use different backup name:
>>>>
>>>> +    backup_filename = u'master-zones-transform-backup.ldif'
>>>>
>>>> Probably something based on time so that different installations' backup do not
>>>> step on each other. You can inspire yourself in other backup files we create:
>>>>
>>>> # ll /var/lib/ipa/backup/
>>>> total 16
>>>> drwx------. 2 root root 4096 May 30 08:10 ipa-full-2014-05-30-08-10-13
>>>> drwx------. 2 root root 4096 May 30 08:11 ipa-full-2014-05-30-08-11-09
>>>> drwx------. 2 root root 4096 May 30 08:13 ipa-full-2014-05-30-08-13-21
>>>> -rw-r--r--. 1 root root 3441 Jun 24 16:25 master-zones-transform-backup.ldif
>>> Is this better: forward-zones-transform-%datetime%.ldif ?
>>
>> It is. I would do something like "dns-forward-zones-backup-YYYY-MM-DD.ldif to
>> match existing files.
>>
>> Martin
> 
> Updated patches attached
> 

1) This is not how you compare bool singletons in Python:

+        if state == False:
+            # no upgrade is needed
+            return (False, False, [])

Rather use "state is False"

Besides that, the upgrade worked fine for me (after I applied my patch 471. So
if you change that one issue, I am willing to ack.

Martin