Yair Zaslavsky has posted comments on this change.
Change subject: engine-manage-domain is missing details on error reports.
......................................................................
Patch Set 4: (4 inline comments)
Some comments.
Good work on commenting inside the code.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/kerberos/ConfigurationProvider.java
Line 102: + " -p " + engineConfigProperties);
Line 103: int retVal = engineConfigProcess.waitFor();
Line 104: if (retVal != 0) {
Line 105: throw new
ManageDomainsResult(ManageDomainsResultEnum.FAILED_SETTING_CONFIGURATION_VALUE_FOR_OPTION,
Line 106: "Verify Connection to Db, Check if DB service
is running",
I would rephrase -
"Please verify the following:\n1. Your database credentials are valid.\n2. The
database machine is accessible.\n3. The database service is running"
Line 107: enumValue.name());
Line 108: }
Line 109: } catch (Throwable e) {
Line 110: throw new
ManageDomainsResult(ManageDomainsResultEnum.FAILED_SETTING_CONFIGURATION_VALUE_FOR_OPTION_WITH_DETAILS,
Line 107: enumValue.name());
Line 108: }
Line 109: } catch (Throwable e) {
Line 110: throw new
ManageDomainsResult(ManageDomainsResultEnum.FAILED_SETTING_CONFIGURATION_VALUE_FOR_OPTION_WITH_DETAILS,
Line 111: "Verify Connection to Db, Check if DB service is
running",
Please extract this "Verify...." to constant - you use it twice at the code.
Line 112: new String[] { enumValue.name(), e.getMessage()
});
Line 113: } finally {
Line 114: disposePassFile(passFile);
Line 115: }
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/kerberos/ManageDomains.java
Line 120: try {
Line 121: utilityConfiguration = new
ManageDomainsConfiguration(configFilePath);
Line 122: } catch (ConfigurationException e) {
Line 123: throw new
ManageDomainsResult(ManageDomainsResultEnum.FAILED_READING_CONFIGURATION,
Line 124: "Verify that postgresql service is up and check
DB configurations", e.getMessage());
Why not use the same string as at ConfigurationProvider? You can create a class
with some constants (public static final String ... ) and use these constants
here and in ConfigurationProvider
Line 125: }
Line 126:
Line 127: try {
Line 128: daoImpl = new ManageDomainsDAOImpl();
....................................................
Commit Message
Line 3: AuthorDate: 2012-08-15 09:41:40 +0300
Line 4: Commit: Yaniv Bronhaim <[email protected]>
Line 5: CommitDate: 2012-08-15 15:28:04 +0300
Line 6:
Line 7: engine-manage-domain is missing details on error reports.
Some comments on title:
engine-manage-domain should be engine-manage-domains.
Please add (#818479) at the end of the title
Line 8:
Line 9: https://bugzilla.redhat.com/show_bug.cgi?id=818479
Line 10:
Line 11: Added variable of default message, so if error occurred during
--
To view, visit http://gerrit.ovirt.org/7202
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If21f117208a53a80ebfefbbe4f6a0c188e35a3bd
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
