Alona Kaplan has posted comments on this change. Change subject: engine, webadmin: refactor manage networks flow ......................................................................
Patch Set 31: (19 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 removing it? 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. i.e- before the 'return false;'? Line 36: return true; Line 37: } Line 38: } Line 39: Line 56: return findPermissionCheckSubjects(networkCluster.getClusterId(), networkCluster, actionType); Line 57: } Line 58: Line 59: public List<PermissionSubject> findPermissionCheckSubjects( Line 60: Guid clusterId, Why do you need the 'clusterId' parameter? You can use networkCluster.getClusterId() (in case networkCluster is null, you can pass null). Line 61: NetworkCluster networkCluster, Line 62: VdcActionType actionType) { Line 63: Line 64: final List<PermissionSubject> permissions = new ArrayList<>(); Line 69: VdcObjectType.Network, Line 70: actionType.getActionGroup())); Line 71: Line 72: // require permissions on cluster the network is attached to Line 73: if (networkExists(clusterId, networkCluster)) { IIUC, this code is a legacy code from the time AttachNetworkToVdsGroup was used by the ui for updating existing networks. Since today updating existing networks via attach command is blocked by a canDoAction, this code is redundant. Line 74: permissions.add(new PermissionSubject(clusterId, Line 75: VdcObjectType.VdsGroups, Line 76: ActionGroup.CONFIGURE_CLUSTER_NETWORK)); Line 77: } Line 77: } Line 78: return permissions; Line 79: } Line 80: Line 81: private boolean networkExists(Guid clusterId, NetworkCluster networkCluster) { I think- "isNetworkClusterExist" is a better name. Line 82: final List<NetworkCluster> networks = networkClusterDao.getAllForCluster(clusterId); Line 83: for (NetworkCluster nc : networks) { Line 84: if (nc.getNetworkId().equals(networkCluster.getNetworkId())) { Line 85: return true; Line 78: return permissions; Line 79: } Line 80: Line 81: private boolean networkExists(Guid clusterId, NetworkCluster networkCluster) { Line 82: final List<NetworkCluster> networks = networkClusterDao.getAllForCluster(clusterId); why not- networkClusterDao.get(networkCluster.getId()); Line 83: for (NetworkCluster nc : networks) { Line 84: if (nc.getNetworkId().equals(networkCluster.getNetworkId())) { Line 85: return true; Line 86: } 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) 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 attachNetworkToHosts was better". Or at least, "attachLabeledNetwork" 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 Line 132: parameters = transformToList(networkClusters, networkClusterToParameterTransformer); Line 133: return runMultipleInternalCommandsSynchronously(actionType, parameters); Line 134: } Line 135: Line 137: sequencially s/sequencially/sequentially 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? "${action} ${type}. Network cluster ${NetworkCluster} appears more then once." You can override- setActionMessageParameters() 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 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 can add it to a utility class. 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? 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? 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? 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? 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 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: 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- NotPredicate. Line 1226: Line 1227: private final IPredicate<T> predicate; Line 1228: Line 1229: private ReversePredicate(IPredicate<T> predicate) { -- 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 <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
