On 10/29/2015 03:01 PM, Martin Babinsky wrote:
On 10/29/2015 01:28 PM, Tomas Babej wrote:


On 10/29/2015 01:10 PM, Petr Vobornik wrote:
On 10/29/2015 11:19 AM, Martin Babinsky wrote:
Petr^2 and Tomas were not happy by the way
https://fedorahosted.org/freeipa/ticket/5175 was handled initially, so
here is a patch that tries to amend some of the issues.




IMHO the original text was good.

Tomas, why is the huge blob thing in exception bad?

Issues with new code/text.

1. it is supported but not for domain level != 0:
self.log.error("Using replica files to set up IPA replicas is not "
+                           "supported."
+            )

2. format() is not needed:
+            self.log.info(
+                "To create a replica, you must promote an existing "
+                "IPA client.".format(domain_level=domain_level)
+            )

3. I don't like that the exception text says 'requires', which might
imply something to be done - lower domain level - which is not possible.
"allowed only" might be better.


Just changing RuntimeError to InvalidDomainLevelError would be fine with
me since the MIN_DOMAIN_LEVEL was already changed to DOMAIN_LEVEL_0.


I was fine with the old text too. In my opinion exceptions should not be
used to handle detailed instructions to the user, they should be used to
briefly summarize the gist of the error that occurred.

If not bad practice, it's at least quite unconventional. There are
better mechanisms to handle the detailed instructions spanning over
multiple lines (with newlines hardcoded) down to the user, such as using
the logger with sufficient level.

So I would approach this issue by just dumping the formatted
UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE down the logger at the error level and
then raising the InvalidDomainLevelError exception with the brief
reasoning.

Tomas

OK i seemed to misunderstand your concerns. Attaching updated patch.


ACK

Pushed to master: 4d94367006287ed0a04c092a7b86096518cf5b8c
--
Petr Vobornik

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

Reply via email to