Juan Hernandez has posted comments on this change. Change subject: restapi: Implementation for GuestOsInfo and Timezone reporting ......................................................................
Patch Set 4: (9 comments) https://gerrit.ovirt.org/#/c/41813/4/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3309: <xs:element name="utc_offset" type="xs:int" minOccurs="1" maxOccurs="1"/> Line 3310: </xs:sequence> Line 3311: </xs:complexType> Line 3312: Line 3313: <xs:complexType name="GuestOs"> Try to avoid abbreviations: GuestOperatingSystem for the type and guest_operating_system for the element. Line 3314: <xs:sequence> Line 3315: <xs:element name="arch" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3316: <xs:element name="codename" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3317: <xs:element name="distribution" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3311: </xs:complexType> Line 3312: Line 3313: <xs:complexType name="GuestOs"> Line 3314: <xs:sequence> Line 3315: <xs:element name="arch" type="xs:string" minOccurs="1" maxOccurs="1"/> We use "architecture" in other places, consider using the same name here as well. Line 3316: <xs:element name="codename" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3317: <xs:element name="distribution" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3318: <xs:element name="kernel_version" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3319: <xs:element name="type" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3312: Line 3313: <xs:complexType name="GuestOs"> Line 3314: <xs:sequence> Line 3315: <xs:element name="arch" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3316: <xs:element name="codename" type="xs:string" minOccurs="1" maxOccurs="1"/> What is the difference between "name" and "codename"? Don't we need a "name"? Line 3317: <xs:element name="distribution" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3318: <xs:element name="kernel_version" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3319: <xs:element name="type" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3320: <xs:element name="version" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3314: <xs:sequence> Line 3315: <xs:element name="arch" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3316: <xs:element name="codename" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3317: <xs:element name="distribution" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3318: <xs:element name="kernel_version" type="xs:string" minOccurs="1" maxOccurs="1"/> May we want to report other kernel parameters in the future (command line, for example)? In that case It may be better to introduce a new "Kernel" complex type with nested elements. Line 3319: <xs:element name="type" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3320: <xs:element name="version" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3321: </xs:sequence> Line 3322: </xs:complexType> Line 3315: <xs:element name="arch" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3316: <xs:element name="codename" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3317: <xs:element name="distribution" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3318: <xs:element name="kernel_version" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3319: <xs:element name="type" type="xs:string" minOccurs="1" maxOccurs="1"/> What is "type"? Windows, Linux, etc? In that case consider naming it "family", as libosinfo does. Line 3320: <xs:element name="version" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3321: </xs:sequence> Line 3322: </xs:complexType> Line 3323: Line 3316: <xs:element name="codename" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3317: <xs:element name="distribution" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3318: <xs:element name="kernel_version" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3319: <xs:element name="type" type="xs:string" minOccurs="1" maxOccurs="1"/> Line 3320: <xs:element name="version" type="xs:string" minOccurs="1" maxOccurs="1"/> Consider using the existing "Version" complex type to represent version numbers. Line 3321: </xs:sequence> Line 3322: </xs:complexType> Line 3323: Line 3324: <xs:complexType name="GuestInfo"> https://gerrit.ovirt.org/#/c/41813/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java: Line 407: Line 408: final boolean hasGuestOsVersion = entity.getGuestOsVersion() != null && !entity.getGuestOsVersion().isEmpty(); Line 409: final boolean hasTimezoneName = entity.getGuestOsTimezoneName() != null && !entity.getGuestOsTimezoneName().isEmpty(); Line 410: if (hasGuestOsVersion) { Line 411: GuestOs os = (new GuestOs()); Take into account that the "os" property may have been populated before, and here it will be completely replaced. In general it is better to check and create it only if it isn't already created: GuestOs os = model.getGuestOs(); if (os == null) { os = new GuestOs(); model.setGuestOs(os); } os.setWhatever(...); ... Line 412: os.setArch(entity.getGuestOsArch().name()); Line 413: os.setCodename(entity.getGuestOsCodename()); Line 414: os.setDistribution(entity.getGuestOsDistribution()); Line 415: os.setKernelVersion(entity.getGuestOsKernelVersion()); Line 413: os.setCodename(entity.getGuestOsCodename()); Line 414: os.setDistribution(entity.getGuestOsDistribution()); Line 415: os.setKernelVersion(entity.getGuestOsKernelVersion()); Line 416: os.setVersion(entity.getGuestOsVersion()); Line 417: os.setType(entity.getGuestOsType().name()); The names of backend enums can't be directly copied to the output of the RESTAPI, as they may change without notice and cause a backwards compatibility problem. Usually we create a RESTAPI specific enum type, and map the values. Look at how DiskInterface is used inside DiskMapper, for example. Line 418: model.setGuestOs(os); Line 419: } Line 420: Line 421: if (hasTimezoneName) { Line 418: model.setGuestOs(os); Line 419: } Line 420: Line 421: if (hasTimezoneName) { Line 422: model.setGuestTimeZone(new TimeZone()); Same, try to create the TimeZone only if it isn't already created. Line 423: model.getGuestTimeZone().setName(entity.getGuestOsTimezoneName()); Line 424: model.getGuestTimeZone().setUtcOffset(entity.getGuestOsTimezoneOffset()); Line 425: } Line 426: -- To view, visit https://gerrit.ovirt.org/41813 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I492d42248858fc653918a33e972734bd82be1eec Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
