Alona Kaplan has posted comments on this change.
Change subject: webadmin: Rewrote Setup Networks operation logic
......................................................................
Patch Set 6: (9 inline comments)
Nice!
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkOperationFactory.java
Line 71: return NetworkOperation.DETACH_NETWORK;
Line 72: }
Line 73: }
Line 74: }
Line 75: // binary operation joining items together - in most cases
valid iff their networks comply
s/iff/if
Line 76: else if (op2 instanceof NetworkInterfaceModel) {
Line 77: NetworkInterfaceModel dst = (NetworkInterfaceModel) op2;
Line 78:
Line 79: // first collect the networks into one set
Line 96: }
Line 97:
Line 98: // go over the networks and check whether they comply, if
not - the reason is important
Line 99: boolean vlanFound = false;
Line 100: String vmNetwork = null;
s/vmNetwork/nonVlanVmNetwork
Line 101: int nonVlanCounter = 0;
Line 102: for (LogicalNetworkModel network : networks) {
Line 103: if (!network.isManaged()) {
Line 104: if (op1 instanceof LogicalNetworkModel) {
Line 130: return
NetworkOperation.NULL_OPERATION_TOO_MANY_NON_VLANS;
Line 131: } else if (vmNetwork != null && vlanFound) {
Line 132: if (op1 instanceof NetworkInterfaceModel) {
Line 133: dst.setCulpritNetwork(vmNetwork);
Line 134: }
Consider adding if (op1 instanceof LogicalNetworkModel). Just to make sure the
code will work in the future.
Line 135: return
NetworkOperation.NULL_OPERATION_VM_WITH_VLANS;
Line 136: }
Line 137: }
Line 138:
Line 141: return NetworkOperation.ATTACH_NETWORK;
Line 142: } else if (op1 instanceof BondNetworkInterfaceModel) {
Line 143: if (op2 instanceof BondNetworkInterfaceModel) {
Line 144: return NetworkOperation.JOIN_BONDS;
Line 145: } else if (op2 instanceof NetworkInterfaceModel) {
This "if" is redundant. op2 is instanceof NetworkInterfaceModel for sure.
Line 146: return NetworkOperation.EXTEND_BOND_WITH;
Line 147: }
Line 148: } else if (op1 instanceof NetworkInterfaceModel) {
Line 149: if (op2 instanceof BondNetworkInterfaceModel) {
Line 147: }
Line 148: } else if (op1 instanceof NetworkInterfaceModel) {
Line 149: if (op2 instanceof BondNetworkInterfaceModel) {
Line 150: return NetworkOperation.ADD_TO_BOND;
Line 151: } else if (op2 instanceof NetworkInterfaceModel) {
Same as the previous comment.
Line 152: return NetworkOperation.BOND_WITH;
Line 153: }
Line 154: }
Line 155: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/SetupNetworksJoinBondsModel.java
Line 13: import org.ovirt.engine.ui.uicommonweb.models.EntityModel;
Line 14: import
org.ovirt.engine.ui.uicommonweb.models.hosts.network.BondNetworkInterfaceModel;
Line 15: import org.ovirt.engine.ui.uicompat.ConstantsManager;
Line 16:
Line 17: public class SetupNetworksJoinBondsModel extends
SetupNetworksBondModel {
Could find SetupNetworksBondModel.java in the patch. Where was it introduced?
Line 18:
Line 19: private List<Entry<String, EntityModel>> bondOptions = new
ArrayList<Entry<String, EntityModel>>();
Line 20: private Map<String, Entry<String, EntityModel>> pairForBondOption
= new HashMap<String, Entry<String, EntityModel>>();
Line 21:
Line 55: }
Line 56:
Line 57: private String getBondOptionForPair(Entry<String, EntityModel>
pair) {
Line 58: String res = pair.getKey();
Line 59: if (res.equals("custom")) { //$NON-NLS-1$
I prefer- if ("custom".equals(res))
Line 60: res = (String) pair.getValue().getEntity();
Line 61: }
Line 62: return res;
Line 63: }
Line 56:
Line 57: private String getBondOptionForPair(Entry<String, EntityModel>
pair) {
Line 58: String res = pair.getKey();
Line 59: if (res.equals("custom")) { //$NON-NLS-1$
Line 60: res = (String) pair.getValue().getEntity();
Please change to return (String) pair.getValue().getEntity() instead if reusing
'res' (it is more readable).
Line 61: }
Line 62: return res;
Line 63: }
Line 64:
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/Constants.java
Line 1671:
Line 1672: @DefaultStringValue("Cannot have more than one non-VLAN-tagged
network on a single interface.")
Line 1673: String nullOperationTooManyNonVlans();
Line 1674:
Line 1675: @DefaultStringValue("Cannot have a VM network and VLAN-tagged
networks on a single interface.")
should be- "Cannot have a non-VLAN-tagged VM network and VLAN-tagged networks
on a single interface."
Line 1676: String nullOperationVmWithVlans();
Line 1677:
Line 1678: @DefaultStringValue("Unassigned Logical Networks panel")
Line 1679: String unassignedLogicalNetworksPanel();
--
To view, visit http://gerrit.ovirt.org/13775
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea5793221d295fa0bd73fc1ed988c90df8fee7c8
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
