[Freeipa-devel] [PATCHES 0227-0229] Server upgrade: introduce ipa-server-upgrade command

[Jan Cholasta]

Hi,

Dne 15.4.2015 v 16:26 Martin Basti napsal(a):
> https://fedorahosted.org/freeipa/ticket/4904
>
> Patches attached.
>
> Also ipa-upgradeconfig part is called as a subprocess. This will be
> removed after installer modifications.
>
> This patch may cause temporal upgrade issues (corner cases), until
> installer part will be finished.
>
> If somebody will be hit by them, please use --skip-version-check for
> ipactl and ipa-server-upgrade.

Regarding that option vs. --force: I think the common assumption is that 
--force ignores *all* non-fatal errors, but you break that assumption in 
ipactl. IMO --force should both ignore errors in service startup *and* 
skip version check, and a new option should be added to just ignore 
errors in service startup (e.g. --ignore-service-failures).

ipa-server-upgrade should probably also have --force, even if it does 
the same thing as --skip-version-check, again because --force is common.


This is a weird API:

+        if data_upgrade.badsyntax:
+            raise admintool.ScriptError(
+                'Bad syntax detected in upgrade file(s).', 1)
+        elif data_upgrade.upgradefailed:
+            raise admintool.ScriptError('IPA upgrade failed.', 1)
+        elif data_upgrade.modified:
+            self.log.info('Data update complete')
+        else:
+            self.log.info('Data update complete, no data were modified')

Why does not IPAUpgrade raise errors instead?


+class IPAVersionError(Exception):
+    pass
+
+class PlatformMismatchError(IPAVersionError):
+    pass
+
+class DataUpgradeRequiredError(IPAVersionError):
+    pass
+
+class DataInNewerVersionError(IPAVersionError):
+    pass

I don't like the "IPA" in "IPAVersionError", it does not tell you much 
about what kind of version is that. Also data version errors should only 
tell you what is wrong, not how you fix it. IMO better names for these 
would be e.g. "UpgradeVersionError", "UpgradePlatformError", 
"UpgradeDataOlderVersionError", "UpgradeDataNewerVersionError". Similar 
for store_ipa_version and check_ipa_version.


Why is it not an error if there is no version in check_ipa_version? IMO 
it should, even if you then ignore the exception most of the time.


Honza

-- 
Jan Cholasta