Alon Bar-Lev has posted comments on this change.
Change subject: oVirt Node Upgrade: Support N configuration
......................................................................
Patch Set 7: (5 inline comments)
Much better! now we can work on the details... some initial comments.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetoVirtISOsQuery.java
Line 43: RpmVersion vdsOsVersion = getOvirtOsVersion();
Line 44: List<RpmVersion> availableISOsList = new
ArrayList<RpmVersion>();
Line 45: OVirtNodeInfo[] infoData = OVirtNodeInfo.get();
Line 46:
Line 47: for (OVirtNodeInfo info : infoData) {
Any reason to hold variable for one time?
for (OVirtNodeInfo info : OVirtNodeInfo.get()) {
Line 48: File directory = new File(info.path);
Line 49:
Line 50: if (directory.isDirectory()) {
Line 51: log.infoFormat("Looking for list of ISOs in [{0}]",
info.path);
Line 44: List<RpmVersion> availableISOsList = new
ArrayList<RpmVersion>();
Line 45: OVirtNodeInfo[] infoData = OVirtNodeInfo.get();
Line 46:
Line 47: for (OVirtNodeInfo info : infoData) {
Line 48: File directory = new File(info.path);
Any reason why not to set info.path as File within OVirtNodeInfo and drop this?
Line 49:
Line 50: if (directory.isDirectory()) {
Line 51: log.infoFormat("Looking for list of ISOs in [{0}]",
info.path);
Line 52: log.infoFormat("regex pattern for ISOs: [{0}]",
info.pattern);
Line 49:
Line 50: if (directory.isDirectory()) {
Line 51: log.infoFormat("Looking for list of ISOs in [{0}]",
info.path);
Line 52: log.infoFormat("regex pattern for ISOs: [{0}]",
info.pattern);
Line 53: List<String> listOfIsoFiles =
getListOfIsoFiles(directory, info);
this should not be here... list files only if required.
if you pass info, why do you pass directory?
Line 54: log.infoFormat("List of ISOs: {0}", listOfIsoFiles);
Line 55:
Line 56: if ((listOfIsoFiles != null) &&
(!listOfIsoFiles.isEmpty())) {
Line 57: File[] ovirtVersionFiles =
filterOvirtFiles(directory, isoVersionPattern);
Line 52: log.infoFormat("regex pattern for ISOs: [{0}]",
info.pattern);
Line 53: List<String> listOfIsoFiles =
getListOfIsoFiles(directory, info);
Line 54: log.infoFormat("List of ISOs: {0}", listOfIsoFiles);
Line 55:
Line 56: if ((listOfIsoFiles != null) &&
(!listOfIsoFiles.isEmpty())) {
No need for extra '()'
if (listOfFiles != null && !listOfIsoFiles.isEmpty) {
}
but anyway... it is not required, as you can drop the nesting and handle this
condition within filterOvirtFiles, just return empty array.
So basically I expect:
for (OVirtNodeInfo info : OVirtNodeInfo.get()) {
for (File versionFile : filterOvirtFiles(info)) {
I think there is single iso per version file... why we need another
loop?
}
}
Line 57: File[] ovirtVersionFiles =
filterOvirtFiles(directory, isoVersionPattern);
Line 58:
Line 59: for (File versionFile : ovirtVersionFiles) {
Line 60: try {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
Line 73: String pathRepo = null;
Line 74:
Line 75: if (StringUtils.isNotBlank(isoFile)) {
Line 76: for (OVirtNodeInfo infoData: Info) {
Line 77: pathRepo = infoData.path + "/" + isoFile;
new File(base, extra)
Line 78: File path = new File(pathRepo);
Line 79: log.infoFormat("Validating ISOs path: {0}", pathRepo);
Line 80: if (path.exists()) {
Line 81: _isoFullPath = pathRepo;
--
To view, visit http://gerrit.ovirt.org/14756
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfb9dc5d0dc8780b519107acbe0ae866831f782c
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Michael Burns <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
