Dne 29.4.2015 v 08:45 Martin Kosek napsal(a):
On 04/29/2015 07:34 AM, Jan Cholasta wrote:
Dne 27.4.2015 v 18:23 David Kupka napsal(a):
On 04/27/2015 04:45 PM, Martin Basti wrote:
On 27/04/15 13:38, Martin Basti wrote:
On 23/04/15 12:55, Martin Basti wrote:
On 21/04/15 10:31, Martin Basti wrote:
On 21/04/15 08:12, Jan Cholasta wrote:
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).
Originally I used --force option to skip detection, but there was
objections against it on list.
However, to have option --force, which set true for both
--ignore-service-failures and --skip-version-check options, might be
better.
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?
For historical reasons, I can investigate what would break this
change, I will send it in separate patch.
+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.
Ok.
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.
I can raise error in that case and ignore the exception.
Honza
Martin^2
Updated patches attached.
Updated patches attached
--
Martin Basti
Updated patch attached
Looks good to me and works as expected. Honza, are you OK with the patches?
Some nitpicks:
The command line tool class should be named "ServerUpgrade" rather than
"IPAServerUpgrade" for consistency with others.
The deprecated --debug option should not be used in new commands.
Why is --debug option deprecated? I thought we wanted to deprecate --verbose
option as --debug is used in most our CLI tools. Well, except ipa-ldap-updated
which for some reasons marks --debug as deprecated. It does not matter now,
given the command is removed/changed.
AdminTool provides --debug as a deprecated alias for --verbose when a
subclass requests it. It seems the decision to deprecate --debug was
already made back when AdminTool was introduced, so let's trust that
decision.
--
Jan Cholasta
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
