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

NOTOK

46

rename e

Cosmetic

NOTOK

52

Must end with .zip

Cosmetic

NOTOK

416

Add exception to the log.trace statement

Cosmetic

NOTOK

349

Add exception to log.trace statement

Cosmetic

NOTOK

335

Replace comment with "// Construct filename"

Cosmetic

NOTOK

329-330

Language: Starting applications on machine:

Cosmetic

NOTOK

269-270

Language: Killing all applications on machine:

Cosmetic

NOTOK

566

Instead of equalsIgnoreCase("") use isEmpty instead

Cosmetic

NOTOK

516

Add exception to the log.trace statement

Cosmetic

NOTOK

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

NOTOK

232

Change to elemList.isEmpty()

Cosmetic

NOTOK

210

Add to log: "Null returned"

Cosmetic

NOTOK

187

Add the following to log: "The entire XML-branch is returned as a string"

Cosmetic

NOTOK

417

Missing validation of content and path

Cosmetic

NOTOK

397

Missing validation of content

Cosmetic

NOTOK

293

change to if (curElems.isEmpty()) {

Cosmetic

NOTOK

161-164

Add this information to the javadoc of the method

Cosmetic

NOTOK

106-108

The f should be replaced with f.getAbsolutePath() Furthermore, the message is duplicated

Cosmetic

NOTOK

367

Cannot validate non-objects, and arg Value is not a String

Cosmetic

NOTOK

73

rename as subtreeRoot, and change the documentation accordingly

Cosmetic

NOTOK

100-101

The f should be replaced with f.getAbsolutePath() Furthermore, the message is duplicated

Cosmetic

NOTOK

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

NOTOK

35

This is => These are

Cosmetic

NOTOK

37

This is => These are

Cosmetic

NOTOK

92-94

Unnecessary else

Cosmetic

NOTOK

98-100

Unnecessary else

Cosmetic

NOTOK

104-106

Unnecessary else

Cosmetic

NOTOK

Comments on file 'trunk/src/dk/netarkivet/deploy/EvaluateConfigFile.java', revision 664

Lines

Description

Classification

Status

123-124

put f.getAbsolutePath inside parenthesis

Cosmetic

NOTOK

137-140

Add to javadoc: Null element represents in this context that no settings branch exists for the current instance

Cosmetic

NOTOK

98-99

Probably use the error message: Error occurred during evaluation

Cosmetic

NOTOK

119-120

put f.getAbsolutePath inside parenthesis

Cosmetic

NOTOK

59

Missing validation

Cosmetic

NOTOK

148

formatting

Cosmetic

NOTOK

Comments on file 'trunk/src/dk/netarkivet/deploy/DeployConfiguration.java', revision 656

Lines

Description

Classification

Status

66

Should be deployConfigName instead.

Cosmetic

NOTOK

89

resetDir will never be null

Cosmetic

NOTOK

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

NOTOK

144-149

Wrong logmessage (copy-pasted). Throw exception with a more proper message

Cosmetic

NOTOK

106

Wrong check: resetDir cannot be null

Cosmetic

NOTOK

84

Rename e

Cosmetic

NOTOK

450-451

Why different message in the log.trace and the IOFailure message

Cosmetic

NOTOK

508-509

Instead of returning "" throw an exception.

Minor

NOTOK

455-456

Why different message in the log.trace and the System.out.println message

Cosmetic

NOTOK

513-522

We ignore the case, when we check the username/password pairs up agains the first username/password

Minor

NOTOK

313

Wrong spelling

Cosmetic

NOTOK

297

Replace with if (!dirs.isEmpty())

Cosmetic

NOTOK

47

wrong comment taken from KBDOMS code

Cosmetic

NOTOK

342

Wrong spelling

Cosmetic

NOTOK

518

This has nothing to do with Heritrix (copy-paste error)

Cosmetic

NOTOK

731

Something missing here?

Cosmetic

NOTOK

586-595

We still ignore any case differencese

Minor

NOTOK

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

NOTOK

281

Error is not unknown. See system.out.println below

Cosmetic

NOTOK

316-318

Delete uncommented code

Cosmetic

NOTOK

265-267

Delete uncommented code

Cosmetic

NOTOK

276-277

Different messages used in the log message, and the exception.

Cosmetic

NOTOK

875

Replace !dir.equalsIgnoreCase("") with !dir.isEmpty()

Cosmetic

NOTOK

326-334

Different messages

Cosmetic

NOTOK

General

A lot of comments refer to our specific test environment Replace by pseudo code in the javadoc of the method itself

Cosmetic

NOTOK

Comments on file 'trunk/src/dk/netarkivet/deploy/DeployApplication.java', revision 656

Lines

Description

Classification

Status

52-53

rename as deployConfigFile

Cosmetic

NOTOK

49

Rename itConfig as "deployConfig"

Cosmetic

NOTOK

82

mailReciever => mailreceiver

Cosmetic

NOTOK

158

Spelling. Check secPolicyFileName and retrieve the corresponding file.

Cosmetic

NOTOK

125

Rename "itConfigName" as "deployConfigName"

Cosmetic

NOTOK

104-110

Maybe redundant?

Cosmetic

NOTOK

192

Remove comment

Cosmetic

NOTOK

161

..and retrieve the corresponding file. (multiple places)

Cosmetic

NOTOK

314-319

Should maybe print out what file it couldn't find: logPropFile.getAbsolutePath

Cosmetic

NOTOK

342-347

Should maybe print out what file it couldn't find: dbFile.getAbsolutePath

Cosmetic

NOTOK

252-256

Should maybe print out what file it couldn't find

Cosmetic

NOTOK

283-288

Should maybe print out what file it couldn't find: secPolicyFile.getAbsolutePath

Cosmetic

NOTOK

417-421

Add after line 419: System.err.Printl("But actually " + changes.length + " arguments was given");

Cosmetic

NOTOK

427-428

What does changes[X] represent? Make local variables for these changes[X]

Cosmetic

NOTOK

351-357

The default setting for the resetArgument is probably n/no. Should probably be mentioned in the javadoc

Cosmetic

NOTOK

379-395

Mention in javadoc, that if evaluateArgument is null, evaluation is skipped.

Cosmetic

NOTOK

481-482

Not entirely correct. Anything other than y/yes/n/no or their Uppercase equivalents results in an errormessage. Rewrite message.

Cosmetic

NOTOK

464-485

Add line "final hasArg = true;" And then replace all copies of "true" with "hasArg"

Cosmetic

NOTOK

497-499

The parsing problems are thrown away. Write errormessage.

Cosmetic

NOTOK

75-81

Some of the optional arguments are missing from the javadoc

Cosmetic

NOTOK

Comments on file 'trunk/src/dk/netarkivet/deploy/PhysicalLocation.java', revision 656

Lines

Description

Classification

Status

73

rename

Cosmetic

NOTOK

94

Improper check. resetDir cannot ever be null

Cosmetic

NOTOK

188-192

Add to javadoc: These scripts will only on UNIX machines

Cosmetic

NOTOK

214-216

The "echo" + StringUtils.repeat... can be replaced by a helper method public String writeDashLine()

Cosmetic

NOTOK

131

Rename Constants.PHYSICAL_LOCATION_NAME_ATTRIBUTES to the same without the last S

Cosmetic

NOTOK

141-143

Throw exception instead of just log, and set the name to "".

Minor

NOTOK

250-252

The error is not unknown

Cosmetic

NOTOK

245-247

Different messages

Cosmetic

NOTOK

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

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