Change in vdsm[master]: contrib: Simple jsonrpc client

piotr.kliczewski at gmail.com piotr.kliczewski at gmail.com
Tue Jan 19 08:03:27 UTC 2016


Piotr Kliczewski has posted comments on this change.

Change subject: contrib: Simple jsonrpc client
......................................................................


Patch Set 7:

(8 comments)

https://gerrit.ovirt.org/#/c/35181/7/contrib/jsonrpc
File contrib/jsonrpc:

Line 25: methods and parameters. The user should consult the schema to construct request
Line 26: that make sense. Future version should parse the schema and provide online
Line 27: help.
Line 28: 
Line 29: Invoking simple methods:
> The :: suffix is intentional, this is rst format for a code block, please k
What is its value?
Line 30: 
Line 31:     # jsonrpc Host.getVMList
Line 32:     ['b3f6fa00-b315-4ad4-8108-f73da817b5c5']
Line 33: 


Line 38: 
Line 39: For invokinng methods with many or complex parameters, you can read the
Line 40: parameters from a file:
Line 41: 
Line 42:     # jsonrpc VM.create -f vm-create
> We like to send json files, not some non-standard key=value format.
key-value format is how we want to pass params. Having a complete message looks like bad idea. We know the method and we want to pass args. I think that we do not need more.
Line 43:     ...
Line 44: 
Line 45: It is also possible to read parameters from standard input, creating complex
Line 46: parameters interactively:


Line 46: parameters interactively:
Line 47: 
Line 48:     # jsonrpc StorageDomain.activate -f -
Line 49:     storagedomainID=75ab40e3-06b1-4a54-a825-2df7a40b93b2,
Line 50:     storagepoolID=b3f6fa00-b315-4ad4-8108-f73da817b5c5
> The previous format was better. Using simple arguments we cannot express js
We can have multiline args but we should not keep a complete message in a file.
Line 51:     (Press Ctrl + D to finish)
Line 52: 
Line 53: """
Line 54: 


Line 100:                 pretty_print(resp.result)
Line 101:         finally:
Line 102:             client.close()
Line 103:     except Exception as e:
Line 104:         fail("Connection failed: %s" % e)
> The jsonrpc changes are good, but the original error handling was much bett
Will improve
Line 105: 
Line 106: 
Line 107: def option_parser():
Line 108:     parser = argparse.ArgumentParser()


Line 109:     parser.add_argument('-a', '--host', dest="host", default="localhost",
Line 110:                         help='host address (default localhost)')
Line 111:     parser.add_argument('method', help='remote method name')
Line 112:     group = parser.add_mutually_exclusive_group()
Line 113:     group.add_argument('-p', '--params', help='params [param=value ...]')
> We don't wan to use -p, using the params as key=value pairs on the command 
There was code where we checked whether we provided file or params directly. It was written to be exclusive so I used mutual exclusion group which requires a param to be optional param. In that way the check is done by argparse instead of doing it manually.
Line 114:     group.add_argument('-f', '--file', dest='file',
Line 115:                        help="read method parameters from json file. Set to"
Line 116:                             " '-' to read from standard input")
Line 117:     return parser


Line 151:         keys.sort()
Line 152:         for element in keys:
Line 153:             representation = pprint.pformat(resp[element]).replace(
Line 154:                 '\n', '\n\t' + ' ' * len(element + ' = ') + '\t')
Line 155:             print('\t' + '\t%s = %s' % (element, representation))
> Why do we need this? pprint.pprint knows how to format dicts nicely
Done
Line 156:     else:
Line 157:         pprint.pprint(resp)
Line 158: 
Line 159: 


Line 153:             representation = pprint.pformat(resp[element]).replace(
Line 154:                 '\n', '\n\t' + ' ' * len(element + ' = ') + '\t')
Line 155:             print('\t' + '\t%s = %s' % (element, representation))
Line 156:     else:
Line 157:         pprint.pprint(resp)
> The previous way to pretify things was better - json format is much better 
We do not get json response. It is already processed at this point.
Line 158: 
Line 159: 
Line 160: def fail(msg):
Line 161:     sys.stderr.write("%s: %s\n" % (os.path.basename(sys.argv[0]), msg))


https://gerrit.ovirt.org/#/c/35181/7/lib/yajsonrpc/stompreactor.py
File lib/yajsonrpc/stompreactor.py:

Line 557:         )
Line 558:     )
Line 559: 
Line 560: 
Line 561: def SimpleClient(host, port=54321, ssl=True):
> Lets separate this change, looks nice addition regardless of the client.
Done
Line 562:     """
Line 563:     Returns JsonRpcClient able to receive jsonrpc messages and notifications.
Line 564:     It is required to provide a host where we want to connect, port and whether
Line 565:     we want to use ssl (True by default). Other settings use defaults and if


-- 
To view, visit https://gerrit.ovirt.org/35181
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6273eabf6f3601602659d1e4e748d8025ae8084
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt at midokura.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Jenkins CI RO
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list