On 01/12/2015 02:28 PM, Jan Cholasta wrote:

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.


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):
fails with:


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

Freeipa-devel mailing list

Reply via email to