[Freeipa-devel] [PATCH] 387 Fix validation of ipa-restore options

[Jan Cholasta]

Dne 13.1.2015 v 13:01 Petr Vobornik napsal(a):
> On 01/12/2015 02:28 PM, Jan Cholasta wrote:
>> Hi,
>>
>> the attached patch fixes <https://fedorahosted.org/freeipa/ticket/4797>.
>>
>> Note that --data with data-only backup and --logs-only with data-only
>> restore are deliberately ignored and considered no-op.
>>
>> Honza
>>
>
> 1. I'm not sure how relative path to backup dir should work:
>
> I have a backup: /var/lib/ipa/backup/ipa-data-2015-01-13-10-53-06/
>
> Absolute path works great. But:
> -  ipa-data-2015-01-13-10-53-06
> fails with `ipa-restore: error: must provide path to backup directory`
>
> Ugly hybrid (I was in home dir):
>      ../../var/lib/ipa/backup/ipa-data-2015-01-13-10-53-06/
> fails with:
>
> /var/lib/ipa/backup/../../var/lib/ipa/backup/ipa-data-2015-01-13-10-53-06/header
>
>
> I.e., dir name won't pass
>       if not os.path.isdir(self.args[0]):
> and the second concatenation is weird.

man ipa-restore says:

     Only the name of the backup needs to be passed in, not the full 
path. Backups are stored in a subdirectory in /var/lib/ipa/backup. If a 
backup is in another location then the full path must be provided.

This was validated wrong, fixed.

>
> 2. Data backup cannot be done because the first 'for' never breaks
>
> +                for instance in instances:
> +                    for backend in backends:
> +                        db_dir = (paths.SLAPD_INSTANCE_DB_DIR_TEMPLATE %
> +                                  (instance, backend))
> +                        if os.path.exists(db_dir):
> +                            break
> +                else:
> +                    raise admintool.ScriptError(
> +                        "Cannot restore a data backup into an empty
> system")

Oops, fixed.

>
> 3. When #2 fixed, data backup with --no-logs doesn't raise warning as
> requested in ticket.

IMO such a warning does not make sense. You request no logs, you get no 
logs, I don't see anything worth warning about here.

>
> 4. Since backup type is detected automatically(--data doesn't have to be
> specified for data restore), I guess that the ticket requirement: `Here
> expecting an error message that --online option can only provided along
> with --data option. ` is invalid, right?
>
> Man page states that --online requires --data option, which is not true.

Fixed in man page.

>
> 5. Nitpick: imho option validation should be done before temp dir creation.

Fixed.

>
> 6. pep8, fixing them is up to you:
>
> ./ipaserver/install/ipa_restore.py:150:5: E129 visually indented line
> with same indent as next logical line
> ./ipaserver/install/ipa_restore.py:184:13: E128 continuation line
> under-indented for visual indent

Fixed.

Updated patch attached.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-387.1-Fix-validation-of-ipa-restore-options.patch
Type: text/x-patch
Size: 13591 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150113/5df7d186/attachment.bin>