Adam Litke has posted comments on this change. Change subject: schema: Updates based on the current code base ......................................................................
Patch Set 3: (10 comments) Responded to some comments. http://gerrit.ovirt.org/#/c/23125/3/vdsm/gluster/vdsmapi-gluster-schema.json File vdsm/gluster/vdsmapi-gluster-schema.json: Line 587: # Line 588: # @volumeName: Gluster volume name Line 589: # Line 590: # @nfs: #optional Line 591: # > I checked gluster api and it was there. I have no clue what it means. Maybe Please try to track someone down and let's get this fixed right away in this patch. Line 592: # Returns: Line 593: # Gluster volume status Line 594: # Line 595: # Since: 4.10.3 Line 891: # Line 892: # @brickList: List of bricks Line 893: # Line 894: # @replicaCount: #optional Line 895: # > I do not know what it is but I saw it in the gluster api so I added to sche You can probably use git-blame to figure out who wrote the function. You can ask them for more information and then update this patch accordingly. Line 896: # Returns: Line 897: # Status information of remove brick operation Line 898: # Line 899: # Since: 4.10.3 http://gerrit.ovirt.org/#/c/23125/3/vdsm_api/vdsmapi-schema.json File vdsm_api/vdsmapi-schema.json: Line 1687 Line 1688 Line 1689 Line 1690 Line 1691 > ? There are a couple of items in the documented enum values that aren't in the actual enum. We should make sure we are consistent. Line 1831 Line 1832 Line 1833 Line 1834 Line 1835 > This change was done by Saggi. I do not know what he meant here. Then it should probably be split out into another patch. Is it related to the rest of your work in this patch series? Line 1836 Line 1837 Line 1838 Line 1839 Line 1840 > As above. I think that Saggi can explain. If it needs explanation by a different author then that is a clear sign that this part needs to be broken out into an independent patch. Line 1051: # Line 1052: # @MIXED: The device consists of a mix of iSCSI and FibreChannel paths Line 1053: # Line 1054: # Since: 4.10.0 Line 1055: # Values allowed : 0, 1, 2, 3, 4, 6, 7, 8, -1 > This is missing information that I missed during implementation. The values OK, but can we assign valid names to them and define some constants in vdsm for them? This string is not helpful at all to an API consumer as is. Line 1056: ## Line 1057: {'enum': 'BlockDeviceType', 'data': ['iSCSI', 'FCP', 'MIXED']} Line 1058: Line 1059: ## Line 1675: # @Iso: The Storage Domain is used for storing ISO images Line 1676: # @Backup: The Storage Domain is used for import and export of disk images Line 1677: # Line 1678: # Since: 4.10.0 Line 1679: # As in the engine Master(1), Data(2), ISO(3), ImportExport(4), Image(5), Unknown(6) > As above. The values are based on the engine enum which is used during call This is better because you provide the mapping between the number and name. I would still add he missing values to the enum itself though. Line 1680: ## Line 1681: {'enum': 'StorageDomainImageClass', Line 1682: 'data': ['Unknown', 'Data', 'Iso', 'Backup']} Line 1683: Line 3043: # @status: State of the VM Line 3044: # Line 3045: # @clientIp: The IP address of the client connected to the display Line 3046: # Line 3047: # Since: 4.10.0 > I can update but need to know more info what is the correct version. Since you're adding something new, just use the next likely release. If you look at the output of 'git tag' it looks like 4.14.1 would be next. Line 3048: ## Line 3049: {'type': 'VMFullList', Line 3050: 'data': {'acpiEnable': 'bool', 'custom': 'StringMap', 'devices': ['VmDevice'], Line 3051: 'display': 'VmDisplayType', 'kvmEnable': 'bool', 'memSize': 'uint', Line 3084: # Returns: Line 3085: # A list of stats for all VMs Line 3086: # Line 3087: # Since: 4.10.0 Line 3088: ## > The version is correct. The api was there but it was not in schema before. ok. Line 3089: {'command': {'class': 'Host', 'name': 'getAllVmStats'}, Line 3090: 'returns': ['VmStats']} Line 3091: Line 3092: ## http://gerrit.ovirt.org/#/c/23125/3/vdsm_api/vdsmapi.py File vdsm_api/vdsmapi.py: Line 230 Line 231 Line 232 Line 233 Line 234 > Not sure. It was done by Saggi when he updated VmDeviceAddressType. Ok, please split out into a separate patch or let Saggi submit it himself. -- To view, visit http://gerrit.ovirt.org/23125 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id3d0d894cc31395510733a33f4f6e15276615a3a Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Timothy Asir <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
