Martin Mucha has posted comments on this change. Change subject: core,restapi: added reported differences element to api.xsd + returning reported differences from GetNetworkAttachment queries ......................................................................
Patch Set 3: (8 comments) + I'm not aware, neither that we want to update reported configurations nor it's even possible. So, based on what you said, it probably should not be subresource. http://gerrit.ovirt.org/#/c/37407/3/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3871: <xs:element ref="host_nic" minOccurs="0" maxOccurs="1"/> Line 3872: <xs:element ref="ip_configuration" minOccurs="0" maxOccurs="1"/> Line 3873: <xs:element ref="properties" minOccurs="0" maxOccurs="1"/> Line 3874: <xs:element name="override_configuration" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 3875: <xs:element name="reported_configurations" type="ReportedConfigurations" minOccurs="0"/> > Define a top level element for this, and use it: Done Line 3876: <xs:element ref="host" minOccurs="0" maxOccurs="1"/> Line 3877: </xs:sequence> Line 3878: </xs:extension> Line 3879: </xs:complexContent> Line 3896: </xs:extension> Line 3897: </xs:complexContent> Line 3898: </xs:complexType> Line 3899: Line 3900: <xs:complexType name="ReportedConfigurations"> > Define a top level element for this complex type: Done Line 3901: <xs:sequence> Line 3902: <xs:element name="in_sync" type="xs:boolean"/> Line 3903: <xs:element name="reportedConfiguration" type="ReportedConfiguration" minOccurs="0" maxOccurs="unbounded"/> Line 3904: </xs:sequence> Line 3898: </xs:complexType> Line 3899: Line 3900: <xs:complexType name="ReportedConfigurations"> Line 3901: <xs:sequence> Line 3902: <xs:element name="in_sync" type="xs:boolean"/> > Is this "in_sync" really a property of the collection, or rather a property needinfo: I'm not sure what's the correct design, I just followed feature page. I'll describe situation and please give me a hint. Network has some QoS setting and so does NIC to which it is attached. Each setting is reported via 'ReportedConfiguration' defined below, reporting to the user name of setting, NIC value and whether this specific setting is in sync with network setting. Plus there's one flag, in_sync on this line, with meaning: "we've evaluated all settings and we consider it generally in-sync" (which can be confusing, since can be something out of sync and result is still in sync). So there are probably two ways: a) these reported configuration are generally considered in-sync (element under ReportedConfigurations) b) network attachment has reported configuration, which is generally considered in-sync. (element on network attachment; this is problematic, since reported configurations aren't passed around always, so placing 'in-sync' element on same level will require it to be optional and there will be 'rule': if we're sending ... we have to also send ...). Due to this complications I'd opt for variant a) which complications I'm not aware of. Line 3903: <xs:element name="reportedConfiguration" type="ReportedConfiguration" minOccurs="0" maxOccurs="unbounded"/> Line 3904: </xs:sequence> Line 3905: </xs:complexType> Line 3906: Line 3899: Line 3900: <xs:complexType name="ReportedConfigurations"> Line 3901: <xs:sequence> Line 3902: <xs:element name="in_sync" type="xs:boolean"/> Line 3903: <xs:element name="reportedConfiguration" type="ReportedConfiguration" minOccurs="0" maxOccurs="unbounded"/> > Define a top level element for this: Done Line 3904: </xs:sequence> Line 3905: </xs:complexType> Line 3906: Line 3907: <xs:complexType name="ReportedConfiguration"> Line 3909: <xs:element name="name" type="xs:string"/> Line 3910: <xs:element name="value" type="xs:string"/> Line 3911: <xs:element name="in_sync" type="xs:boolean"/> Line 3912: </xs:sequence> Line 3913: </xs:complexType> > This looks pretty much like a set of properties, except for the "in_sync" f if we're ok with sacrificing explicit element names, element count, and type of in_sync it should not pose any problem. It's only used to pass data to the user, so it should be ok. Should I use Properties then? Line 3914: Line 3915: <xs:element name="ip_configuration" type="IpConfiguration"/> Line 3916: Line 3917: <xs:complexType name="IpConfiguration"> http://gerrit.ovirt.org/#/c/37407/3/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ReportedConfigurationsMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ReportedConfigurationsMapper.java: Line 2: Line 3: import java.util.List; Line 4: Line 5: import org.ovirt.engine.api.model.ReportedConfiguration; Line 6: import org.ovirt.engine.core.common.businessentities.network.ReportedConfigurations; > When the names of the backend entities and the names of the RESTAPI entitie Done Line 7: Line 8: public class ReportedConfigurationsMapper { Line 9: Line 10: @Mapping(from = ReportedConfigurations.class, to = org.ovirt.engine.api.model.ReportedConfigurations.class) Line 4: Line 5: import org.ovirt.engine.api.model.ReportedConfiguration; Line 6: import org.ovirt.engine.core.common.businessentities.network.ReportedConfigurations; Line 7: Line 8: public class ReportedConfigurationsMapper { > We don't usually create mapper classes for collections, but for entities. S this bring us back to decision where will be 'in_sync' element. If it's part of NetworkAattachment then it's fine and I can do this. But if it's not, then I'd be obliged to process it separately in caller, which seems impractical. Or not? Line 9: Line 10: @Mapping(from = ReportedConfigurations.class, to = org.ovirt.engine.api.model.ReportedConfigurations.class) Line 11: public static org.ovirt.engine.api.model.ReportedConfigurations map(ReportedConfigurations entity, Line 12: org.ovirt.engine.api.model.ReportedConfigurations template) { Line 19: for (ReportedConfigurations.ReportedConfiguration reportedConfiguration : entity.getReportedConfigurationList()) { Line 20: Line 21: ReportedConfiguration conf = new ReportedConfiguration(); Line 22: conf.setInSync(reportedConfiguration.isInSync()); Line 23: conf.setName(reportedConfiguration.getType().getName()); > What if "reportedConfiguration.getType()" returns null? that would be invalid response and shouldn't happen. It's enum, but I've added checks for nullity in code, so if anybody in future tries to smuggle null into here, it will fail before reaching this point. Line 24: conf.setValue(reportedConfiguration.getValue()); Line 25: reportedConfigurationList.add(conf); Line 26: } Line 27: -- To view, visit http://gerrit.ovirt.org/37407 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia7f83d268e250c24c4eeee6bff832426699ad95d Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
