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

[Petr Vobornik]

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.

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

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

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.

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

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