Yevgeny Zaspitsky has posted comments on this change. Change subject: engine, webadmin: refactor manage networks flow ......................................................................
Patch Set 31: (17 comments) http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkClusterPermissionsChecker.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkClusterPermissionsChecker.java: Line 29: Line 30: public boolean checkPermissions(CommandBase<?> command, List<PermissionSubject> permissionCheckSubjects) { Line 31: Line 32: for (PermissionSubject permSubject : permissionCheckSubjects) { Line 33: final List<String> messages = new ArrayList<>(); > The original code had 'messages.clear();' here. What is the reason for remo Done Line 34: if (command.checkSinglePermission(permSubject, messages)) { Line 35: command.getReturnValue().getCanDoActionMessages().addAll(messages); Line 36: return true; Line 37: } Line 31: Line 32: for (PermissionSubject permSubject : permissionCheckSubjects) { Line 33: final List<String> messages = new ArrayList<>(); Line 34: if (command.checkSinglePermission(permSubject, messages)) { Line 35: command.getReturnValue().getCanDoActionMessages().addAll(messages); > Shouldn't you update the messages just in case the permissions check fails. Done Line 36: return true; Line 37: } Line 38: } Line 39: http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterInternalCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToClusterInternalCommand.java: Line 18: public class AttachNetworkToClusterInternalCommand<T extends AttachNetworkToVdsGroupParameter> extends Line 19: NetworkClusterCommandBase<T> { Line 20: Line 21: AttachNetworkToClusterInternalCommand(T parameters) { Line 22: super(parameters, null); > Please call- super(parameters) Done Line 23: } Line 24: Line 25: public AttachNetworkToClusterInternalCommand(T parameters, CommandContext cmdContext) { Line 26: super(parameters, cmdContext); http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java: Line 43: propagateFailure(returnValue); Line 44: } Line 45: } Line 46: Line 47: private void attachLabeledNetworks() { > You didn't replay to my comment on patch-set 22- "I think the previous name Done Line 48: final AttachNetworkToVdsGroupParameter attachNetworkToVdsGroupParameter = getParameters(); Line 49: Line 50: runInternalAction( Line 51: VdcActionType.PropagateLabeledNetworksToClusterHosts, http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/ManageNetworkClustersCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/ManageNetworkClustersCommand.java: Line 127: private boolean runNetworkClusterCommands( Line 128: Collection<NetworkCluster> networkClusters, Line 129: VdcActionType actionType, Line 130: Function<NetworkCluster, ? extends VdcActionParametersBase> networkClusterToParameterTransformer) { Line 131: final List<? extends VdcActionParametersBase> > Please format Done Line 132: parameters = transformToList(networkClusters, networkClusterToParameterTransformer); Line 133: return runMultipleInternalCommandsSynchronously(actionType, parameters); Line 134: } Line 135: Line 137: sequencially > s/sequencially/sequentially Done Line 243: getParameters().getDetachments(), Line 244: getParameters().getUpdates()); Line 245: for (NetworkCluster networkCluster : inputNetworkClusters) { Line 246: if (networkClusterIds.contains(networkCluster.getId())) { Line 247: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DUPLICATE_NETWORK_CLUSTER_INPUT); > What about the replacements? Done Line 248: } else { Line 249: networkClusterIds.add(networkCluster.getId()); Line 250: } Line 251: } http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/PropagateLabeledNetworksToClusterHostsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/PropagateLabeledNetworksToClusterHostsCommand.java: Line 62: isSetupNetworSupported > s/isSetupNetworSupported/isSetupNetworkSupported Done http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterPermissionsChecker.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkClusterPermissionsChecker.java: Line 35: Line 36: /** Line 37: * Checks the user has permissions either on one of the objects at least. Line 38: */ Line 39: public boolean checkPermissions(CommandBase<?> command, List<PermissionSubject> permissionCheckSubjects) { > Since AttachNetworkClusterPermissionsChecker has the same method, maybe you AttachNetworkClusterPermissionsChecker does not use the logic anymore Line 40: final List<String> messages = new ArrayList<>(); Line 41: for (PermissionSubject permSubject : permissionCheckSubjects) { Line 42: messages.clear(); Line 43: if (command.checkSinglePermission(permSubject, messages)) { http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/transformer/NetworkClustersToSetupNetworksParametersTransformerImpl.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/transformer/NetworkClustersToSetupNetworksParametersTransformerImpl.java: Line 64: labelsToNicsByHost.put(nic.getVdsId(), new HashMap<String, VdsNetworkInterface>()); Line 65: } Line 66: Line 67: labelsToNicsByHost.get(nic.getVdsId()).put(network.getLabel(), nic); Line 68: attachNetworksByHost.get(nic.getVdsId()).add(network); > Shouldn't it be outside the for loop? why? Line 69: } Line 70: } Line 71: Line 72: Map<Guid, List<Network>> detachNetworksByHost = new HashMap<>(); Line 84: detachNicsByHost.put(nic.getVdsId(), new ArrayList<VdsNetworkInterface>()); Line 85: } Line 86: Line 87: detachNicsByHost.get(nic.getVdsId()).add(nic); Line 88: detachNetworksByHost.get(nic.getVdsId()).add(network); > Shouldn't it be outside the for loop? why? Line 89: } Line 90: } Line 91: Line 92: return createSetupNetworksParameters(attachNetworksByHost, http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AttachNetworkToVdsGroupParameter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AttachNetworkToVdsGroupParameter.java: Line 35: public Network getNetwork() { Line 36: return _network; Line 37: } Line 38: Line 39: AttachNetworkToVdsGroupParameter() { > Why did you remove the public modifier? I'd prefer it would not exists at all. However I guess that is used by serialization framework. So reducing visibility is the least I can do. Line 40: } http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java: Line 207: AttachNetworkToClusterInternal(707, false, QuotaDependency.NONE), Line 208: AttachNetworkToVdsGroup(708, ActionGroup.ASSIGN_CLUSTER_NETWORK, false, QuotaDependency.NONE), Line 209: DetachNetworkToVdsGroup(709, ActionGroup.ASSIGN_CLUSTER_NETWORK, false, QuotaDependency.NONE), Line 210: UpdateNetworkOnCluster(711, ActionGroup.CONFIGURE_CLUSTER_NETWORK, false, QuotaDependency.NONE), Line 211: > Why not- 712, 713? Done Line 212: DetachNetworkFromClusterInternal(714, false, QuotaDependency.NONE), Line 213: ManageNetworkClusters(715, ActionGroup.ASSIGN_CLUSTER_NETWORK, false, QuotaDependency.NONE), Line 214: Line 215: /** http://gerrit.ovirt.org/#/c/33684/31/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/linq/LinqUtils.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/linq/LinqUtils.java: Line 82: Line 83: return map; Line 84: } Line 85: Line 86: public static <IN, KEY, VALUE> Map<KEY, List<VALUE>> toMultiMap(Collection<IN> collection, Mapper<IN, KEY, VALUE> mapper) { > Please add javadoc Done Line 87: final Map<KEY, List<VALUE>> map = new HashMap<>(); Line 88: for (IN in : collection) { Line 89: final KEY key = mapper.createKey(in); Line 90: final List<VALUE> values; http://gerrit.ovirt.org/#/c/33684/31/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java: > Seems that the changes in this file are not related and not used by the cur Done Line 1: package org.ovirt.engine.ui.uicommonweb; Line 2: Line 3: import java.io.Serializable; Line 4: import java.util.ArrayList; Line 1221: } Line 1222: Line 1223: } Line 1224: Line 1225: private static class ReversePredicate<T> implements IPredicate<T> { > Please be consistent with the naming. In the backend you called it- NotPred Done Line 1226: Line 1227: private final IPredicate<T> predicate; Line 1228: Line 1229: private ReversePredicate(IPredicate<T> predicate) { http://gerrit.ovirt.org/#/c/33684/31/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterNetworkManageModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterNetworkManageModel.java: Line 174: final NetworkCluster networkCluster = new NetworkCluster(); Line 175: Line 176: networkCluster.setClusterId(manageModel.getCluster().getId()); Line 177: networkCluster.setNetworkId(manageModel.getEntity().getId()); Line 178: // networkCluster.setStatus(NetworkStatus.OPERATIONAL); // ??? > Please remove the comment. Done Line 179: copyRoles(manageModel, networkCluster); Line 180: Line 181: return networkCluster; Line 182: } -- To view, visit http://gerrit.ovirt.org/33684 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a0b86d8bb018a6172891cb357a4402cfef135d1 Gerrit-PatchSet: 31 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
