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