12424
Comment:
|
12403
|
Deletions are marked like this. | Additions are marked like this. |
Line 155: | Line 155: |
|| 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 || NOTOK || || 46 || Please rename this variable || Cosmetic || NOTOK || || 89 || Spelling of argument: mailReceiver || Cosmetic || NOTOK || || 116-120 || What does the ones and twos represent here. Replace with constant || Cosmetic || NOTOK || || 208 || The description of the method does not correspond to the name of the method || Cosmetic || NOTOK || || 186 || rename method to properly state its purpose. || Cosmetic || NOTOK || || 69 || missing validation of configSource || Cosmetic || NOTOK || |
|| 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 || |
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 |
NOTOK |
Use constants for strings that are used repeatedly, like "\n", "ssh" etc. |
Cosmetic |
NOTOK |
It is not explicitly mentioned that it is assumed that installation takes place from a Unix machine |
Cosmetic |
NOTOK |
replace "new StringBuilder("")" with "new StringBuilder()" |
Cosmetic |
NOTOK |
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 |
NOTOK |
68 |
Improper errorhandling. Replace with an ArgumentNotValid exception |
Cosmetic |
NOTOK |
95 |
Mention this sideeffect in javadoc of this method |
Cosmetic |
NOTOK |
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 |
NOTOK |
107 |
We are overwriting the name, if it exists already; otherwise it is inserted. |
Cosmetic |
NOTOK |
62 |
Rename e |
Cosmetic |
NOTOK |
164 |
Missing validation of directory |
Cosmetic |
NOTOK |
124-126 |
Remove redundant else-clause |
Cosmetic |
NOTOK |
141 |
Apply => use |
Cosmetic |
NOTOK |
219 |
Missing validation of path |
Cosmetic |
NOTOK |
229 |
Missing validation of path |
Cosmetic |
NOTOK |
57 |
Rename as applicationInstanceId |
Cosmetic |
NOTOK |
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 |
NOTOK |
46 |
Deploy specific parameters |
Cosmetic |
NOTOK |
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 |
NOTOK |
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 |