Bugzilla – Full Text Bug Listing
| Summary: | TPTP needs to provide upgrade functionality | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | jkubasta | ||||||||||
| Component: | TPTP | Assignee: | Joel Cayne <jcayne> | ||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P1 | CC: | paulslau | ||||||||||
| Version: | unspecified | Keywords: | plan | ||||||||||
| Target Milestone: | --- | Flags: | paulslau:
review+
|
||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows 2000 | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
jkubasta
The information that can be provided for the upgrade based on the current SetConfig includes: Java Home, Network access mode, Security enabled and Users allowed to access the Agent Controller. The information can be output to a property file or stored in a hashtable and accessed by the AC at upgrade. Created attachment 90359 [details]
CreateUpgrade class
Attached class contains the functionality for upgrading the AC version for TPTP 4.5.0 to a new install. Process to follow allows for obtaining the installation information from the current install, uninstall and new installation using the data stored (obtained by this class).
(In reply to comment #2) > Created an attachment (id=90359) [details] > CreateUpgrade class > > Attached class contains the functionality for upgrading the AC version for TPTP > 4.5.0 to a new install. Process to follow allows for obtaining the installation > information from the current install, uninstall and new installation using the > data stored (obtained by this class). > Reviewed with comments: -Change $Id: $ to $Id$. -Put the package declaration below the copyright comment. -Add the @author, @version, and @since tags to the class JavaDoc comment. -Define static constants in a constants class/interface for JAVA_PATH or is there a similar constant to SecurityEnabled.TAG, for example. -Instead of walking the DOM (see traverseNode(NodeList), consider querying the exact properties you need since there are only a few (e.g. getElementById(String) or getElementsByTagName(String)). -Do you need getDoc() and getHash() (getPropertyFile(File) should return the map)? -Consider making this class a singleton (privatized constructor) or provide s static method getPropertyFile(File) that returns the map. -Why do you need to store "Agent Controller Configuration" in the map? Created attachment 90748 [details]
CreateUpgrade class - 2
Thanks for reviewing the class Paul, I have made changes based on your comments.
- Updated the Id field
- Moved the package declaration
- Added the JavaDoc tags
- Replaced JAVA_PATH and host with a constant
- Used getElementsByTagName(String) instead of walking the DOM
- Removed getDoc() and getHash() (updated getPropertyFile(File)).
- Made the class a singleton.
- Removed "Agent Controller Configuration"
Paul, can you please review the updated class? Thanks!
(In reply to comment #4) > Created an attachment (id=90748) [details] > CreateUpgrade class - 2 > > Thanks for reviewing the class Paul, I have made changes based on your > comments. > > - Updated the Id field > - Moved the package declaration > - Added the JavaDoc tags > - Replaced JAVA_PATH and host with a constant > - Used getElementsByTagName(String) instead of walking the DOM > - Removed getDoc() and getHash() (updated getPropertyFile(File)). > - Made the class a singleton. > - Removed "Agent Controller Configuration" > > Paul, can you please review the updated class? Thanks! > Reviewed. Looks good. -I would put your full name in the class comment so others can easily identify you. -org.eclipse.tptp.platform.agentcontroller.config.Constants should be included in this patch. -Unless you expect this class to be subclasses, make your protected methods and fields private. -Stylistic comment, I like putting braces around single line blocks (e.g. if, loops, etc.) since it makes it easier to read and less prone to error if other lines are added to the block. -The hash variable should be called properties (or similar) and you should probably type the return value of the getPropertyFile() (maybe called addPropertyFile()) method to Properties. -I think you need getHash() or have parseDocument() return the properties. My original comment was in context of not have the file passed to the singleton accessor (or having a public parseDocument() method). Created attachment 91249 [details]
CreateUpgrade class - 3
Updated based on comments. Thanks Paul.
Patch checked into HEAD. (In reply to comment #6) > Created an attachment (id=91249) [details] > CreateUpgrade class - 3 > > Updated based on comments. Thanks Paul. > Joel, there is a problem with a singleton accessors accepting a parameter that is not used until the parseDocument() method is called. The singleton design pattern only contained shared state (e.g. instance variables) between callers. Passing in a file path that is associated only to the caller breaks the design pattern. For example, caller A passes in file path A1 at time T1, caller B passes in file path B1 at time T2, and caller A calls the parseDocument() method at time T3. To resolve, change the getInstance() method to not accept any parameters and change the parseDocument() method to accept the file path parameter. In addition, all new APIs in non-internal packaged require the @provisional JavaDoc tag (see http://www.eclipse.org/tptp/home/documents/process/development/api_contract.html). Created attachment 91372 [details]
ConfigUpgrade patch
Adds @provisional tag and moves the file path to parse as part of parseDocument().
(In reply to comment #9) > Created an attachment (id=91372) [details] > ConfigUpgrade patch > > Adds @provisional tag and moves the file path to parse as part of > parseDocument(). > Looks nice! Verified, so closing |