= Review (NS-20): Implentation of new Deploy module =
|| Author || Jonas ||
|| Moderator || Jonas ||
|| State || Closed ||
== Objectives ==
{{{
Old module removed, and replaced by this new module, which need review.
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
ELZI: 10
SVC:  10
JOLF: 21
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| The word "IT config" should be replaced with deploy config || Cosmetic || OK ||
|| Use constants for strings that are used repeatedly, like "\n", "ssh" etc. || Cosmetic || OK ||
|| It is not explicitly mentioned that it is assumed that installation takes place from a Unix machine || Cosmetic || OK ||
|| replace "new StringBuilder("")" with "new StringBuilder()" || Cosmetic || OK ||


=== Comments on file 'trunk/src/dk/netarkivet/deploy/LinuxMachine.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 67 || replace linux with constant, or let Machine class contain an OS enum containing at the moment LINUX, WINDOWS || Cosmetic || OK ||
|| 46 || rename e || Cosmetic || OK ||
|| 52 || Must end with .zip || Cosmetic || OK ||
|| 416 || Add exception to the log.trace statement || Cosmetic || OK ||
|| 349 || Add exception to log.trace statement || Cosmetic || OK ||
|| 335 || Replace comment with "// Construct filename" || Cosmetic || OK ||
|| 329-330 || Language: Starting applications on machine: || Cosmetic || OK ||
|| 269-270 || Language: Killing all applications on machine: || Cosmetic || OK ||
|| 566 || Instead of equalsIgnoreCase("") use isEmpty instead || Cosmetic || OK ||
|| 516 || Add exception to the log.trace statement || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/BuildCompleteSettings.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 106 || Mention in javadoc, that this method returns null, if there are problems with this file Or perhaps throw an IOEXception instead? || Cosmetic || OK ||
|| 68 || Improper errorhandling. Replace with an ArgumentNotValid exception || Cosmetic || OK ||
|| 95 || Mention this sideeffect in javadoc of this method || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/XmlStructure.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 264 || Add "." at end of line || Cosmetic || OK ||
|| 232 || Change to elemList.isEmpty() || Cosmetic || OK ||
|| 210 || Add to log: "Null returned" || Cosmetic || OK ||
|| 187 || Add the following to log: "The entire XML-branch is returned as a string" || Cosmetic || OK ||
|| 417 || Missing validation of content and path || Cosmetic || OK ||
|| 397 || Missing validation of content || Cosmetic || OK ||
|| 293 || change to if (curElems.isEmpty()) { || Cosmetic || OK ||
|| 161-164 || Add this information to the javadoc of the method || Cosmetic || OK ||
|| 106-108 || The f should be replaced with f.getAbsolutePath() Furthermore, the message is duplicated || Cosmetic || OK ||
|| 367 || Cannot validate non-objects, and arg Value is not a String || Cosmetic || OK ||
|| 73 || rename as subtreeRoot, and change the documentation accordingly || Cosmetic || OK ||
|| 100-101 || The f should be replaced with f.getAbsolutePath() Furthermore, the message is duplicated || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/Parameters.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 130-142 || Maybe we should also check if tmp has more than one element || Cosmetic || OK ||
|| 35 || This is => These are || Cosmetic || OK ||
|| 37 || This is => These are || Cosmetic || OK ||
|| 92-94 || Unnecessary else || Cosmetic || OK ||
|| 98-100 || Unnecessary else || Cosmetic || OK ||
|| 104-106 || Unnecessary else || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/EvaluateConfigFile.java', revision 664 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 123-124 || put f.getAbsolutePath inside parenthesis || Cosmetic || OK ||
|| 137-140 || Add to javadoc: Null element represents in this context that no settings branch exists for the current instance || Cosmetic || OK ||
|| 98-99 || Probably use the error message: Error occurred during evaluation || Cosmetic || OK ||
|| 119-120 || put f.getAbsolutePath inside parenthesis || Cosmetic || OK ||
|| 59 || Missing validation || Cosmetic || OK ||
|| 148 || formatting || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/DeployConfiguration.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 66 || Should be deployConfigName instead. || Cosmetic || OK ||
|| 89 || resetDir will never be null || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/Application.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 38 || the deploy structure is a somewhat confusing term || Cosmetic || OK ||
|| 107 || We are overwriting the name, if it exists already; otherwise it is inserted. || Cosmetic || OK ||
|| 62 || Rename e || Cosmetic || OK ||
|| 164 || Missing validation of directory || Cosmetic || OK ||
|| 124-126 || Remove redundant else-clause || Cosmetic || OK ||
|| 141 || Apply => use || Cosmetic || OK ||
|| 219 || Missing validation of path || Cosmetic || OK ||
|| 229 || Missing validation of path || Cosmetic || OK ||
|| 57 || Rename as applicationInstanceId || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/Constants.java', revision 664 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 175-181 || How is this information used in the DeployApplication. Add to javadoc: This is the default location of the database || Cosmetic || OK ||
|| 46 || Deploy specific parameters || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/Machine.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 170 || Missing argument validation || Cosmetic || OK ||
|| 144-149 || Wrong logmessage (copy-pasted). Throw exception with a more proper message || Cosmetic || OK ||
|| 106 || Wrong check: resetDir cannot be null || Cosmetic || OK ||
|| 84 || Rename e || Cosmetic || OK ||
|| 450-451 || Why different message in the log.trace and the IOFailure message || Cosmetic || OK ||
|| 508-509 || Instead of returning "" throw an exception. || Minor || OK ||
|| 455-456 || Why different message in the log.trace and the System.out.println message || Cosmetic || OK ||
|| 513-522 || We ignore the case, when we check the username/password pairs up agains the first username/password || Minor || OK ||
|| 313 || Wrong spelling || Cosmetic || OK ||
|| 297 || Replace with if (!dirs.isEmpty()) || Cosmetic || OK ||
|| 47 || wrong comment taken from KBDOMS code || Cosmetic || OK ||
|| 342 || Wrong spelling || Cosmetic || OK ||
|| 518 || This has nothing to do with Heritrix (copy-paste error) || Cosmetic || OK ||
|| 731 || Something missing here? || Cosmetic || OK ||
|| 586-595 || We still ignore any case differencese || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/complete_settings.xml', revision 687 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add SVN-header (see build.xml) Add file-description || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/WindowsMachine.java', revision 665 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 725 || It-config => Deploy configuration || Cosmetic || OK ||
|| 281 || Error is not unknown. See system.out.println below || Cosmetic || OK ||
|| 316-318 || Delete uncommented code || Cosmetic || OK ||
|| 265-267 || Delete uncommented code || Cosmetic || OK ||
|| 276-277 || Different messages used in the log message, and the exception. || Cosmetic || OK ||
|| 875 || Replace !dir.equalsIgnoreCase("") with !dir.isEmpty() || Cosmetic || OK ||
|| 326-334 || Different messages || Cosmetic || OK ||
|| General || A lot of comments refer to our specific test environment Replace by pseudo code in the javadoc of the method itself || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/DeployApplication.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 52-53 || rename as deployConfigFile || Cosmetic || OK ||
|| 49 || Rename itConfig as "deployConfig" || Cosmetic || OK ||
|| 82 || mailReciever => mailreceiver || Cosmetic || OK ||
|| 158 || Spelling. Check secPolicyFileName and retrieve the corresponding file. || Cosmetic || OK ||
|| 125 || Rename "itConfigName" as "deployConfigName" || Cosmetic || OK ||
|| 104-110 || Maybe redundant? || Cosmetic || OK ||
|| 192 || Remove comment || Cosmetic || OK ||
|| 161 || ..and retrieve the corresponding file. (multiple places) || Cosmetic || OK ||
|| 314-319 || Should maybe print out what file it couldn't find: logPropFile.getAbsolutePath || Cosmetic || OK ||
|| 342-347 || Should maybe print out what file it couldn't find: dbFile.getAbsolutePath || Cosmetic || OK ||
|| 252-256 || Should maybe print out what file it couldn't find || Cosmetic || OK ||
|| 283-288 || Should maybe print out what file it couldn't find: secPolicyFile.getAbsolutePath || Cosmetic || OK ||
|| 417-421 || Add after line 419: System.err.Printl("But actually " + changes.length + " arguments was given"); || Cosmetic || OK ||
|| 427-428 || What does changes[X] represent? Make local variables for these changes[X] || Cosmetic || OK ||
|| 351-357 || The default setting for the resetArgument is probably n/no. Should probably be mentioned in the javadoc || Cosmetic || OK ||
|| 379-395 || Mention in javadoc, that if evaluateArgument is null, evaluation is skipped. || Cosmetic || OK ||
|| 481-482 || Not entirely correct. Anything other than y/yes/n/no or their Uppercase equivalents results in an errormessage. Rewrite message. || Cosmetic || OK ||
|| 464-485 || Add line "final hasArg = true;" And then replace all copies of "true" with "hasArg" || Cosmetic || OK ||
|| 497-499 || The parsing problems are thrown away. Write errormessage. || Cosmetic || OK ||
|| 75-81 || Some of the optional arguments are missing from the javadoc || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/PhysicalLocation.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 73 || rename || Cosmetic || OK ||
|| 94 || Improper check. resetDir cannot ever be null || Cosmetic || OK ||
|| 188-192 || Add to javadoc: These scripts will only on UNIX machines || Cosmetic || OK ||
|| 214-216 || The "echo" + StringUtils.repeat... can be replaced by a helper method  public String writeDashLine() || Cosmetic || OK ||
|| 131 || Rename Constants.PHYSICAL_LOCATION_NAME_ATTRIBUTES to the same without the last S || Cosmetic || OK ||
|| 141-143 || Throw exception instead of just log, and set the name to "". || Minor || OK ||
|| 250-252 || The error is not unknown || Cosmetic || OK ||
|| 245-247 || Different messages || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/CreateTestInstance.java', revision 656 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 36-40 || Add to javadoc:  This class is prompted by our need to be able to install and run multiple instances the NetarchiveSuite in our test-environment simultaneously || Cosmetic || OK ||
|| 46 || Please rename this variable || Cosmetic || OK ||
|| 89 || Spelling of argument: mailReceiver || Cosmetic || OK ||
|| 116-120 || What does the ones and twos represent here. Replace with constant || Cosmetic || OK ||
|| 208 || The description of the method does not correspond to the name of the method || Cosmetic || OK ||
|| 186 || rename method to properly state its purpose. || Cosmetic || OK ||
|| 69 || missing validation of configSource || Cosmetic || OK ||