Martin Mucha has posted comments on this change. Change subject: core: Introduce NetworkAttachment entity ......................................................................
Patch Set 19: (2 comments) https://gerrit.ovirt.org/#/c/32411/19/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkAttachment.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkAttachment.java: Line 22: private Guid networkId; Line 23: Line 24: private Guid nicId; Line 25: Line 26: // TODO: Add validation for the name for nic or bond > I'm not sure I understand why do you need this TODO? You just need to check note: all mine TODOs has format: "//TODO MM: .*" Others are not mine todos. I cannot remeber (without looking into following patches) how did I fix this, I'll check this and reply in next comment. There's problem with bean validation, since SetupHostNetworkCommand param class aggregates NetworkAttachment class instances, which has to be validated using CreateEntity, UpdateEntity and RemoveEntity validation groups at a same time, which is not possible. So even if I fix this using bean validation, for SetupHostNetworkCommand it has to be validated (probably) manually as well in can do action. You're right. In nicName can be both nic and bond name and we cannot tell the difference here. Line 27: private String nicName; Line 28: Line 29: private IpConfiguration ipConfiguration; Line 30: private Map<String, String> properties; Line 107: return false; Line 108: if (getClass() != obj.getClass()) Line 109: return false; Line 110: NetworkAttachment other = (NetworkAttachment) obj; Line 111: if (id == null) { > 1. Please use Objects.equals(id, other.id) 'equals' and hashcode are automatically generated methods by IDE. Please tell me which style do you prefer, and I regenerate them. All styles are valid and using IDE generated methods are safer than fiddling with them manually. Maybe post me example what your IDE generates and what you consider correct style and I will use this style everywhere. I cannot answer the question about properties. I don't know how this object will be compared. Since we will migrate to use ORM we should write this carefully. NetworkAttachment differing "only" in properties then can behave wrong (like not updating itself), since it's unchanged. To me, NetworkAttachment differing 'only' in properties is not the same. But as I said, I cannot answer it properly. Line 112: if (other.id != null) Line 113: return false; Line 114: } else if (!id.equals(other.id)) Line 115: return false; -- To view, visit https://gerrit.ovirt.org/32411 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ided81dc2be68dc4c7a9d491697f887cdae477a2c Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
