Michael Pasternak has posted comments on this change. Change subject: restapi: Fix update cluster network (#822631) ......................................................................
Patch Set 1: (1 inline comment) .................................................... File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendClusterNetworkResource.java Line 32: new AttachNetworkToVdsGroupParameter(cluster.getVDSGroup(), map(incoming, map(get())))); > Display command was replaced by the attach command, it is deprecated and not > to be used any more. how possibly VdcActionType.UpdateDisplayToVdsGroup can be replaced with VdcActionType.AttachNetworkToVdsGroup ??? *** UpdateDisplayToVdsGroup used to set single property in existent cluster-network, => display=true *** AttachNetworkToVdsGroup used to attach network to cluster > Attach command is save or update command, why would it need to change? it is save command only in context of attach of *new* network to cluster > > Also during attach you can update many fields that are related to the > attachment > of the network, not only usages, which you cannot update currently, so this > fix > also applies to them. not correct, maybe you can update network fields with this command, but it's true only during *attach* action, using it in other contexts is abuse of this command and as i mentioned before i would expect to see can-do-action: "network already attach to this cluster" on second use of AttachNetworkToVdsGroup on same network+cluster. > I see no reason why REST should care how engine behaves on it's commands > as long as API remains the same, and is the same logical meaning. The idea is > still the same, you update the attachment of network to cluster using this > command, if there is a modelling problem it's in the API itself, not in what > command is being used. the problems are different: 1. by supplying different cluster-id and using update on cluster-network you will attach this network to new cluster, this is *non* RESTfull behaviour and i would not like to have it in api. 2. anyone looking on BackendClusterNetworkResource.update() will think that you can attach network to cluster via update, but this is chicken&egg situation cause BackendClusterNetworkResource would not exist if it was not already attached - this code is awkward ... 3. Backend-wise this is abuse of AttachNetworkToVdsGroup, if update of the fields is the same for attach&&update it should be: UpdateClusterNetwork AttachNetworkToVdsGroup::UpdateClusterNetwork and in AttachNetworkToVdsGroup.execute() you should call super.execute() when you need to update fields. -- To view, visit http://gerrit.ovirt.org/5513 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3ba860433d0a77caad6405fb30b9b29b0af3a847 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Michael Pasternak <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
