Michael Pasternak has posted comments on this change. Change subject: cli: construct object using factory on update, rather than fetching live instance ......................................................................
Patch Set 6: Code-Review-1 (4 comments) thanks .................................................... File src/ovirtcli/command/update.py Line 159: ) Line 160: elif hasattr(resource, 'update'): Line 161: param_obj = self.create_params_obj() Line 162: if hasattr(param_obj, 'id'): Line 163: setattr(param_obj, 'id', getattr(resource, 'id')) as i mentioned in previous patch this is redundant Line 164: resource_class = getattr(brokers, resource.__class__.__name__) Line 165: resource_instance = resource_class(param_obj, resource.context) Line 166: resource_instance = self.update_object_data(resource_instance, opts) Line 167: result = self.execute_method(resource_instance, 'update', opts) Line 162: if hasattr(param_obj, 'id'): Line 163: setattr(param_obj, 'id', getattr(resource, 'id')) Line 164: resource_class = getattr(brokers, resource.__class__.__name__) Line 165: resource_instance = resource_class(param_obj, resource.context) Line 166: resource_instance = self.update_object_data(resource_instance, opts) all this code is not needed, you do not need to create an instance of the broker, just an empty parameters holder from xml.params (as you did in line 161) Line 167: result = self.execute_method(resource_instance, 'update', opts) Line 168: Line 169: else: Line 170: self.error( Line 171: Messages.Error.OBJECT_IS_IMMUTABLE % (args[0], args[1]) Line 172: ) Line 173: self.context.formatter.format(self.context, result) Line 174: Line 175: def create_params_obj(self): 1. i'd accept here a type name rather than relaying on class variable args 2. please make this method private Line 176: """Create an instance of param object.""" Line 177: args = self.arguments Line 178: params_xml_type = ParseHelper.getXmlType(args[0]) Line 179: param_obj = None Line 177: args = self.arguments Line 178: params_xml_type = ParseHelper.getXmlType(args[0]) Line 179: param_obj = None Line 180: if params_xml_type: Line 181: param_obj = params_xml_type.factory() this is not enough, there might be different types based on the parent of the object such as HostNIC/NIC (former has a /host as a parent on 'update nic'), consider using self.resolve_base(opts) to find parent of update candidate and actual type from collection as done at /add command Line 182: return param_obj Line 183: Line 184: def show_help(self): Line 185: """Show help for "update".""" -- To view, visit http://gerrit.ovirt.org/14557 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34a910a7f1ecae5385e2a6dcd2297a65f419bc9c Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine-cli Gerrit-Branch: master Gerrit-Owner: Ravi Nori <[email protected]> Gerrit-Reviewer: Michael Pasternak <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
