Juan Hernandez has posted comments on this change. Change subject: restapi: Implementation for GuestOsInfo and Timezone reporting ......................................................................
Patch Set 6: (6 comments) https://gerrit.ovirt.org/#/c/41813/6/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3311: </xs:complexType> Line 3312: Line 3313: <xs:complexType name="Kernel"> Line 3314: <xs:sequence> Line 3315: <xs:element name="version" type="xs:string" minOccurs="0" maxOccurs="1"/> What about this "version", can we use the "Version" complex type? Line 3316: </xs:sequence> Line 3317: </xs:complexType> Line 3318: Line 3319: <xs:complexType name="GuestOperatingSystem"> https://gerrit.ovirt.org/#/c/41813/6/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 306: seconds -= hours * 3600; Line 307: int minutes = seconds / 60; Line 308: // Format string into this form: +03:00 or -09:30 etc Line 309: return String.format("%s%02d:%02d", value < 0 ? "-" : "+", hours, minutes); Line 310: } It may be good idea to move this logic (and related tests) to a new TimeZoneMapper class. Line 311: Line 312: @Mapping(from = org.ovirt.engine.core.common.businessentities.VM.class, to = VM.class) Line 313: public static VM map(org.ovirt.engine.core.common.businessentities.VM entity, VM template) { Line 314: return map(entity, template, true); Line 414: } Line 415: } Line 416: } Line 417: Line 418: final boolean hasGuestOsVersion = entity.getGuestOsVersion() != null && !entity.getGuestOsVersion().isEmpty(); If the backend used its "Version" class here you could just use the VersionMapper.map method. Line 419: final boolean hasTimezoneName = entity.getGuestOsTimezoneName() != null && !entity.getGuestOsTimezoneName().isEmpty(); Line 420: if (hasGuestOsVersion) { Line 421: GuestOperatingSystem os = (new GuestOperatingSystem()); Line 422: os.setArchitecture(entity.getGuestOsArch().name()); Line 417: Line 418: final boolean hasGuestOsVersion = entity.getGuestOsVersion() != null && !entity.getGuestOsVersion().isEmpty(); Line 419: final boolean hasTimezoneName = entity.getGuestOsTimezoneName() != null && !entity.getGuestOsTimezoneName().isEmpty(); Line 420: if (hasGuestOsVersion) { Line 421: GuestOperatingSystem os = (new GuestOperatingSystem()); No need for the parenthesis, and try to avoid creating elements without checking, you may be discarding previously populated values: GuestOperatingSystem os = model.getGuestOperatingSystem(); if (os == null) { os = new GuestOperatingSystem(); model.setGuestOperatingSystem(os); } ... Line 422: os.setArchitecture(entity.getGuestOsArch().name()); Line 423: os.setCodename(entity.getGuestOsCodename()); Line 424: os.setDistribution(entity.getGuestOsDistribution()); Line 425: os.setKernel(new Kernel()); Line 438: os.getVersion().setBuild(Integer.parseInt(StringUtils.stripStart(parts[2], "0"))); Line 439: } Line 440: if (parts.length > 3) { Line 441: os.getVersion().setRevision(Integer.parseInt(StringUtils.stripStart(parts[3], "0"))); Line 442: } Ideally the backend should return an instance of the "Version" class, so that this parsing is performed outside of the RESTAPI. If that isn't possible then consider moving this code to the "VersionMapper" class. Line 443: } Line 444: catch(NumberFormatException e) { Line 445: // No need to handle the failure, just ignore Line 446: } Line 441: os.getVersion().setRevision(Integer.parseInt(StringUtils.stripStart(parts[3], "0"))); Line 442: } Line 443: } Line 444: catch(NumberFormatException e) { Line 445: // No need to handle the failure, just ignore Don't ignore, at least send a message to the log, including the stack trace. Line 446: } Line 447: os.getVersion().setFullVersion(entity.getGuestOsVersion()); Line 448: os.setFamily(entity.getGuestOsType().name()); Line 449: model.setGuestOperatingSystem(os); -- 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: 6 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
