)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"8d0a9aa0e4991da78d7a1bf1f1cf9b3dfb7dbb51","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch implements OVO for VLAN aware VMs data model."},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"In addition to OVO, this patch also updates DB model to introduce"},{"line_number":12,"context_line":"TrunkPort entity (with port_id as primary key) instead of Trunk.port_id."},{"line_number":13,"context_line":"The reasoning behind this is to allow L2 agents to use"},{"line_number":14,"context_line":"rpc_callbacks\u0027 pull API to fetch Trunk OVOs by only requireing port_id."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a122d0e_2400d733","line":12,"updated":"2016-04-27 13:09:49.000000000","message":"is this the only reason? It seems to me that the tail is wagging the dog, and I honestly don\u0027t think it\u0027s a reasonable thing to do.","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"35c47ab6f0c8956a9113c04dbd86ab3ce3a09e65","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch implements OVO for VLAN aware VMs data model."},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"In addition to OVO, this patch also updates DB model to introduce"},{"line_number":12,"context_line":"TrunkPort entity (with port_id as primary key) instead of Trunk.port_id."},{"line_number":13,"context_line":"The reasoning behind this is to allow L2 agents to use"},{"line_number":14,"context_line":"rpc_callbacks\u0027 pull API to fetch Trunk OVOs by only requireing port_id."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a122d0e_82d271fe","line":12,"in_reply_to":"1a122d0e_1f2362be","updated":"2016-04-27 19:51:49.000000000","message":"that\u0027s not true. You can update the properties of the trunk (parent and children ports) just as easily.\n\nAs far as the rpc_resource is concerned, this can made look whatever we want it to look like","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"c3e8853d2ed5a1140dfb36ba3f22578a9294bf57","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch implements OVO for VLAN aware VMs data model."},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"In addition to OVO, this patch also updates DB model to introduce"},{"line_number":12,"context_line":"TrunkPort entity (with port_id as primary key) instead of Trunk.port_id."},{"line_number":13,"context_line":"The reasoning behind this is to allow L2 agents to use"},{"line_number":14,"context_line":"rpc_callbacks\u0027 pull API to fetch Trunk OVOs by only requireing port_id."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a122d0e_1f2362be","line":12,"in_reply_to":"1a122d0e_2400d733","updated":"2016-04-27 14:30:59.000000000","message":"It\u0027s not the only reason. It also adds more flexibility to the model and for example would allow the Trunk to be detached from the trunk Port without loosing the Trunk definition and contained SubPorts. And it fits nicely with rpc_resources API.","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"9b28a8778c3404877858864c45a2a7a41855c32d","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch implements OVO for VLAN aware VMs data model."},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"In addition to OVO, this patch also updates DB model to introduce"},{"line_number":12,"context_line":"TrunkPort entity (with port_id as primary key) instead of Trunk.port_id."},{"line_number":13,"context_line":"The reasoning behind this is to allow L2 agents to use"},{"line_number":14,"context_line":"rpc_callbacks\u0027 pull API to fetch Trunk OVOs by only requireing port_id."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a122d0e_a53ccae1","line":12,"in_reply_to":"1a122d0e_4b94ed35","updated":"2016-04-28 10:04:53.000000000","message":"Please, disregard the \u0027primary keys as resource_id\u0027s\u0027 part. \u0027trunk_id\u0027 would be an extended attribute of \u0027port\u0027.","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4afa49f6c3a163f4bfa26ecd31f8b0a8f330706b","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch implements OVO for VLAN aware VMs data model."},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"In addition to OVO, this patch also updates DB model to introduce"},{"line_number":12,"context_line":"TrunkPort entity (with port_id as primary key) instead of Trunk.port_id."},{"line_number":13,"context_line":"The reasoning behind this is to allow L2 agents to use"},{"line_number":14,"context_line":"rpc_callbacks\u0027 pull API to fetch Trunk OVOs by only requireing port_id."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a122d0e_ab58218b","line":12,"in_reply_to":"1a122d0e_513f81b3","updated":"2016-04-28 04:30:38.000000000","message":"please see my response to Armando\u0027s comment","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"},{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"92ff38bee18a08e25ad46e9067e29656a3fb7e66","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch implements OVO for VLAN aware VMs data model."},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"In addition to OVO, this patch also updates DB model to introduce"},{"line_number":12,"context_line":"TrunkPort entity (with port_id as primary key) instead of Trunk.port_id."},{"line_number":13,"context_line":"The reasoning behind this is to allow L2 agents to use"},{"line_number":14,"context_line":"rpc_callbacks\u0027 pull API to fetch Trunk OVOs by only requireing port_id."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a122d0e_513f81b3","line":12,"in_reply_to":"1a122d0e_82d271fe","updated":"2016-04-27 23:00:29.000000000","message":"What do we gain from allowing the Trunk to be detached from its trunk Port? A Trunk represents a Port, right? Does it make sense to have a Trunk that isn\u0027t associated with a Port?","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4afa49f6c3a163f4bfa26ecd31f8b0a8f330706b","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch implements OVO for VLAN aware VMs data model."},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"In addition to OVO, this patch also updates DB model to introduce"},{"line_number":12,"context_line":"TrunkPort entity (with port_id as primary key) instead of Trunk.port_id."},{"line_number":13,"context_line":"The reasoning behind this is to allow L2 agents to use"},{"line_number":14,"context_line":"rpc_callbacks\u0027 pull API to fetch Trunk OVOs by only requireing port_id."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a122d0e_4b94ed35","line":12,"in_reply_to":"1a122d0e_82d271fe","updated":"2016-04-28 04:30:38.000000000","message":"Yes, but I intended to avoid updates (on relationship keys \u0026 segmentation details) in favour of immutable objects. And in it\u0027s current state it is not possible to have a detached trunk. Also that model change was actually inspired by [1] :)\n\nI do understand that it is possible to provide any callback to neutron.api.rpc.callbacks.producer.registry, but I thought it would be preferable to use primary keys as resource_id\u0027s.\n\n[1] https://review.openstack.org/#/c/279251/11/neutron/services/trunk/models.py@25","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"25017c802b620824b0ff38e09dea135ede3609c2","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This patch implements OVO for VLAN aware VMs data model."},{"line_number":10,"context_line":""},{"line_number":11,"context_line":"In addition to OVO, this patch also updates DB model to introduce"},{"line_number":12,"context_line":"TrunkPort entity (with port_id as primary key) instead of Trunk.port_id."},{"line_number":13,"context_line":"The reasoning behind this is to allow L2 agents to use"},{"line_number":14,"context_line":"rpc_callbacks\u0027 pull API to fetch Trunk OVOs by only requireing port_id."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1a122d0e_1d577727","line":12,"in_reply_to":"1a122d0e_a53ccae1","updated":"2016-05-02 17:41:35.000000000","message":"I suggest you revert the model/schema changes and work within what we already defined. I look forward to the rebased patch!","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"}],"neutron/objects/trunk.py":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"3c27c3a5e1e7804dfc5cf4b46f62bad30b25d6d0","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2016 Mirantis, Inc."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_8934924a","line":1,"updated":"2016-05-04 18:15:10.000000000","message":"I\u0027d rather have all details pertaining one feature in one place, so I would personally move this under neutron/services/trunk/objects but I appreciate this is not possible due to some weird magic happening during tests.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"945e83355f7687f4d5999c9b3a41cf76d72f39a8","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2016 Mirantis, Inc."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_52585a31","line":1,"in_reply_to":"1a122d0e_8934924a","updated":"2016-05-04 21:28:10.000000000","message":"I believe this is a usage requirement of oslo versioned objects, but I\u0027m not sure why: http://docs.openstack.org/developer/oslo.versionedobjects/usage.html#create-objects-subdirectory-and-a-base-py-inside-it. All other versioned objects for neutron are in this directory as well.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"9e6b3fc3648d88745eb5cbb609699f9e787d7525","unresolved":false,"context_lines":[{"line_number":130,"context_line":"    def delete(self):"},{"line_number":131,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":132,"context_line":"            self.load_synthetic_db_fields()"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"            for sub_port in self.sub_ports:"},{"line_number":135,"context_line":"                sub_port.delete()"},{"line_number":136,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_5594ba35","line":133,"updated":"2016-05-05 04:41:25.000000000","message":"There\u0027s a potential race here if ports are attached/detached between load and self.delete. I\u0027m thinking of 2 options:\n\n(a) lock trunk (row in the DB) for the duration of delete/attach/detach [probably safest approach]\n\n(b) rely on on-delete-cascade constraint (for attached ports) and ignore exceptions from sub_port.delete (if any) [better performance, simple]\n\nMost likely will do (b) as part of this review and leave (a) for later.","commit_id":"84408a3ebb8b3c3ce4e077481c801e4313489823"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"9e6b3fc3648d88745eb5cbb609699f9e787d7525","unresolved":false,"context_lines":[{"line_number":139,"context_line":"            super(Trunk, self).delete()"},{"line_number":140,"context_line":""},{"line_number":141,"context_line":"    def _create_sub_port(self, port_id, segmentation_type, segmentation_id):"},{"line_number":142,"context_line":"        # TODO(ivc): allow segmentation_id to be None (i.e. auto-generated)"},{"line_number":143,"context_line":"        sub_port \u003d SubPort(self._context, port_id\u003dport_id, trunk_id\u003dself.id,"},{"line_number":144,"context_line":"                           segmentation_type\u003dsegmentation_type,"},{"line_number":145,"context_line":"                           segmentation_id\u003dsegmentation_id)"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_952132a1","line":142,"range":{"start_line":142,"start_character":10,"end_line":142,"end_character":19},"updated":"2016-05-05 04:41:25.000000000","message":"that will be covered by another change (outside of this review)","commit_id":"84408a3ebb8b3c3ce4e077481c801e4313489823"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"618917c3ddf203f079dd1571b8c0e304bb632dfc","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        Loads the :class:`Trunk` object corresponding to the specified trunk"},{"line_number":72,"context_line":"        port."},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        Note: this method does not load the :field:`sub_ports` field value for"},{"line_number":75,"context_line":"        performance reasons. It is up to the caller to load"},{"line_number":76,"context_line":"        :field:`sub_ports` by using :method:`load_synthetic_db_fields` when"},{"line_number":77,"context_line":"        necessary."},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"        :param context: The context within which to perform the action"},{"line_number":80,"context_line":"        :param port_id: :class:`str` -- port identifier"}],"source_content_type":"text/x-python","patch_set":5,"id":"1a122d0e_47b0828b","line":77,"range":{"start_line":74,"start_character":0,"end_line":77,"end_character":18},"updated":"2016-05-05 12:38:57.000000000","message":"That is a false statement and is to be removed in the next PS.","commit_id":"f384aa068e4190a67fa9e25d4d0f3764c59fa94d"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"b580046329196995606a7977c71651ad1fa479a7","unresolved":false,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    db_model \u003d models.Trunk"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"    primary_keys \u003d [\u0027id\u0027]"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    fields \u003d {"},{"line_number":57,"context_line":"        \u0027id\u0027: obj_fields.UUIDField(),"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2f476a1e","line":54,"updated":"2016-05-09 12:40:52.000000000","message":"This is declared in base class. This line can be removed.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    db_model \u003d models.Trunk"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"    primary_keys \u003d [\u0027id\u0027]"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    fields \u003d {"},{"line_number":57,"context_line":"        \u0027id\u0027: obj_fields.UUIDField(),"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_7f153f9a","line":54,"in_reply_to":"dab17558_2f476a1e","updated":"2016-05-11 06:59:21.000000000","message":"fixed","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":60,"context_line":"        \u0027sub_ports\u0027: obj_fields.ListOfObjectsField(\u0027SubPort\u0027),"},{"line_number":61,"context_line":"    }"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    fields_no_update \u003d [\u0027id\u0027, \u0027tenant_id\u0027, \u0027port_id\u0027]"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    synthetic_fields \u003d [\u0027sub_ports\u0027]"},{"line_number":66,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_da8495d5","line":63,"updated":"2016-05-09 16:47:21.000000000","message":"no need to specify keys in fields_no_update, so \u0027id\u0027 can be removed","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":60,"context_line":"        \u0027sub_ports\u0027: obj_fields.ListOfObjectsField(\u0027SubPort\u0027),"},{"line_number":61,"context_line":"    }"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    fields_no_update \u003d [\u0027id\u0027, \u0027tenant_id\u0027, \u0027port_id\u0027]"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    synthetic_fields \u003d [\u0027sub_ports\u0027]"},{"line_number":66,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_bf0b277c","line":63,"in_reply_to":"dab17558_da8495d5","updated":"2016-05-11 06:59:21.000000000","message":"fixed","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        :raises TrunkPortNotFound: if the specified port is not a trunk port"},{"line_number":80,"context_line":"        \"\"\""},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"        with db_api.autonested_transaction(context.session):"},{"line_number":83,"context_line":"            trunks \u003d cls.get_objects(context, port_id\u003dport_id)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"            if not trunks:"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_dad2d5d1","line":82,"updated":"2016-05-09 16:47:21.000000000","message":"no need to start a transaction here, get_objects will take care of that","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        :raises TrunkPortNotFound: if the specified port is not a trunk port"},{"line_number":80,"context_line":"        \"\"\""},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"        with db_api.autonested_transaction(context.session):"},{"line_number":83,"context_line":"            trunks \u003d cls.get_objects(context, port_id\u003dport_id)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"            if not trunks:"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_9f8583d0","line":82,"in_reply_to":"dab17558_dad2d5d1","updated":"2016-05-11 06:59:21.000000000","message":"see create_trunk function. the transaction makes sense now","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":86,"context_line":"                raise t_exc.TrunkPortNotFound(port_id\u003dport_id)"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2a794a94","line":89,"updated":"2016-05-06 18:25:57.000000000","message":"port id is unique on the trunk. I don\u0027t expect this except be raised.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":86,"context_line":"                raise t_exc.TrunkPortNotFound(port_id\u003dport_id)"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_84f30673","line":89,"in_reply_to":"dab17558_2a794a94","updated":"2016-05-06 23:19:39.000000000","message":"Neither do I, but I\u0027d normally prefer to cover all cases, no matter how crazy they may sound at the moment.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"a5aa0d7a0ce6cc11aaa608b5ab7cb15cf047b622","unresolved":false,"context_lines":[{"line_number":86,"context_line":"                raise t_exc.TrunkPortNotFound(port_id\u003dport_id)"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_873783af","line":89,"in_reply_to":"dab17558_84f30673","updated":"2016-05-07 00:21:17.000000000","message":"It is crazy indeed and you should not do it. It\u0027s an extra code path to be tested for no good reason.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":86,"context_line":"                raise t_exc.TrunkPortNotFound(port_id\u003dport_id)"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_1f723311","line":89,"in_reply_to":"dab17558_873783af","updated":"2016-05-11 06:59:21.000000000","message":"fixed","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2aee6a22","line":91,"updated":"2016-05-06 18:25:57.000000000","message":"are you sure we can use get_object() instead, I don\u0027t see how we can return multiple trunks (as a list) under the current logical constraints.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_dfb5a7ee","line":91,"in_reply_to":"dab17558_0ff426f0","updated":"2016-05-09 16:47:21.000000000","message":"Maybe it\u0027s time to introduce a new method in the base class or to change get_object. get_object behaves differently from get_objects, I agree it\u0027s confusing. Luckily the OVO stuff is still WIP, we can change it as much as needed.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_6421c2fb","line":91,"in_reply_to":"dab17558_2aee6a22","updated":"2016-05-06 23:19:39.000000000","message":"Yes, get_object was the first thing I tried, but it can only be used with what is declared in Trunk.primary_keys.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"a5aa0d7a0ce6cc11aaa608b5ab7cb15cf047b622","unresolved":false,"context_lines":[{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_c7418b0d","line":91,"in_reply_to":"dab17558_6421c2fb","updated":"2016-05-07 00:21:17.000000000","message":"I see, good to know.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"b580046329196995606a7977c71651ad1fa479a7","unresolved":false,"context_lines":[{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_0ff426f0","line":91,"in_reply_to":"dab17558_c7418b0d","updated":"2016-05-09 12:40:52.000000000","message":"Yes, the get_object is intended to fetch only one object using the primary keys - this makes the unique constraint and deterministic behavior. It was done to replace get_by_id() method, since in Neutron we can have multiple primary keys for DB records.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":88,"context_line":"            if len(trunks) \u003e 1:"},{"line_number":89,"context_line":"                raise RuntimeError(_LE(\"Trunk.port_id is not unique.\"))"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_ff280f9c","line":91,"in_reply_to":"dab17558_dfb5a7ee","updated":"2016-05-11 06:59:21.000000000","message":"@Rossella, I don\u0027t think it is worth a new method. But we could probably add \u0027unique_keys\u0027 to NeutronDbObject and allow \u0027get_object\u0027 to accept them in addition to \u0027primary_key\u0027.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"},{"line_number":95,"context_line":"        \"\"\"Create :class:`Trunk` object for a given `port_id`."},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        This is a helper method that verifies if the specified port can be"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_4ad00e54","line":94,"updated":"2016-05-06 18:25:57.000000000","message":"If you intend to allow the specification of an undetermined number of parameters, naming this create_for_port_id doesn\u0027t make much sense.\n\nWhat if we called it build() or construct()?","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"},{"line_number":95,"context_line":"        \"\"\"Create :class:`Trunk` object for a given `port_id`."},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        This is a helper method that verifies if the specified port can be"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_91ce73b2","line":94,"in_reply_to":"dab17558_2f6af380","updated":"2016-05-09 16:47:21.000000000","message":"why don\u0027t you simply override create ?","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"},{"line_number":95,"context_line":"        \"\"\"Create :class:`Trunk` object for a given `port_id`."},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        This is a helper method that verifies if the specified port can be"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_0276586f","line":94,"in_reply_to":"dab17558_4ad00e54","updated":"2016-05-11 06:59:21.000000000","message":"@Armando, I think having both \u0027create\u0027 and \u0027build\u0027/\u0027construct\u0027 could be a bit confusing. Instead, I\u0027m replacing it with \u0027create_trunk\u0027 module-level function. That should make more sense (same with get_trunk).","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"},{"line_number":95,"context_line":"        \"\"\"Create :class:`Trunk` object for a given `port_id`."},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        This is a helper method that verifies if the specified port can be"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2f6af380","line":94,"in_reply_to":"dab17558_4ad00e54","updated":"2016-05-06 23:19:39.000000000","message":"I\u0027ve just followed the \u0027create\u0027 semantics but forced port_id to be specified, hence the name and the \u0027fields\u0027. On the second thought, \u0027fields\u0027 do not make much sense as of now, and if we\u0027d need to add more fields to Trunk, we\u0027d make them explicit anyway.\n\nThanks I\u0027ll remove the \u0027fields\u0027 argument.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":91,"context_line":"            return trunks[0]"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"    @classmethod"},{"line_number":94,"context_line":"    def create_for_port_id(cls, context, port_id, **fields):"},{"line_number":95,"context_line":"        \"\"\"Create :class:`Trunk` object for a given `port_id`."},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        This is a helper method that verifies if the specified port can be"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_22b43c33","line":94,"in_reply_to":"dab17558_91ce73b2","updated":"2016-05-11 06:59:21.000000000","message":"@Rossella, I tried that before, but BaseObjectIfaceTestCase did not expect that at all. As far as I understand, create/delete/get_object(s) API is supposed to serve the same role as, say, oslo_db or SA does, but should not care about business logic.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        :param context: The context within which to perform the action"},{"line_number":103,"context_line":"        :param port_id: :class:`str` -- port identifier"},{"line_number":104,"context_line":"        :param fields: Other `fields` to pass to :class:`Trunk`\u0027s constructor"},{"line_number":105,"context_line":"                       (reserved for future versions)"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        :returns: :class:`Trunk` -- new trunk object"},{"line_number":108,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_0a6d6698","line":105,"updated":"2016-05-06 18:25:57.000000000","message":"I\u0027d rather not use such a polymorphic interface if we can help it. So consider  being explicit about the parameters used here. Have you consider including a list of support objects, so that a trunk can be created on the fly with its parent and children? That would support an API call that contains both parent port id and the list of subports.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        :param context: The context within which to perform the action"},{"line_number":103,"context_line":"        :param port_id: :class:`str` -- port identifier"},{"line_number":104,"context_line":"        :param fields: Other `fields` to pass to :class:`Trunk`\u0027s constructor"},{"line_number":105,"context_line":"                       (reserved for future versions)"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        :returns: :class:`Trunk` -- new trunk object"},{"line_number":108,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_620be4d6","line":105,"in_reply_to":"dab17558_07739381","updated":"2016-05-11 06:59:21.000000000","message":"Moved it outside of the class, hope it makes more sense now. And added \u0027sub_ports\u0027 as you suggested.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        :param context: The context within which to perform the action"},{"line_number":103,"context_line":"        :param port_id: :class:`str` -- port identifier"},{"line_number":104,"context_line":"        :param fields: Other `fields` to pass to :class:`Trunk`\u0027s constructor"},{"line_number":105,"context_line":"                       (reserved for future versions)"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        :returns: :class:`Trunk` -- new trunk object"},{"line_number":108,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_8f603f37","line":105,"in_reply_to":"dab17558_0a6d6698","updated":"2016-05-06 23:19:39.000000000","message":"see above comment about \u0027fields\u0027.\n\nRegarding subport list - yes, I did consider that. I can add another helper method that would call create \u0026 attach if you say it is worth it. But I\u0027d like to keep create_for_port_id as it is anyway. What do you say?","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"a5aa0d7a0ce6cc11aaa608b5ab7cb15cf047b622","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        :param context: The context within which to perform the action"},{"line_number":103,"context_line":"        :param port_id: :class:`str` -- port identifier"},{"line_number":104,"context_line":"        :param fields: Other `fields` to pass to :class:`Trunk`\u0027s constructor"},{"line_number":105,"context_line":"                       (reserved for future versions)"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"        :returns: :class:`Trunk` -- new trunk object"},{"line_number":108,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_07739381","line":105,"in_reply_to":"dab17558_8f603f37","updated":"2016-05-07 00:21:17.000000000","message":"This is very much dependent on how you intend to use it, do you already have an example? Otherwise perhaps it\u0027s best to settle for this signature like the following?\n\n def build(cls, context, parent_port, sub_ports\u003dNone);\n\ncan parent_port and sub_ports objects themselves?","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":117,"context_line":"        trunk \u003d cls(context, port_id\u003dport_id, tenant_id\u003dtenant_id, **fields)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        with db_api.autonested_transaction(context.session):"},{"line_number":120,"context_line":"            with trunk._check_create():"},{"line_number":121,"context_line":"                trunk.create()"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        return trunk"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2ad82a53","line":120,"updated":"2016-05-06 18:25:57.000000000","message":"I\u0027d expand the _check_create method here for readability, unless you see it reused already somewhere else.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":117,"context_line":"        trunk \u003d cls(context, port_id\u003dport_id, tenant_id\u003dtenant_id, **fields)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        with db_api.autonested_transaction(context.session):"},{"line_number":120,"context_line":"            with trunk._check_create():"},{"line_number":121,"context_line":"                trunk.create()"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        return trunk"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_75096e95","line":120,"in_reply_to":"dab17558_2ad82a53","updated":"2016-05-06 23:19:39.000000000","message":"I\u0027ve deliberately moved it outside, so that the \u0027normal case\u0027 code is separated from the error/exception handling, exactly for the readability reasons :)","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":117,"context_line":"        trunk \u003d cls(context, port_id\u003dport_id, tenant_id\u003dtenant_id, **fields)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        with db_api.autonested_transaction(context.session):"},{"line_number":120,"context_line":"            with trunk._check_create():"},{"line_number":121,"context_line":"                trunk.create()"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        return trunk"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_eeb1da90","line":120,"in_reply_to":"dab17558_670a5fd4","updated":"2016-05-09 16:47:21.000000000","message":"+1","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"a5aa0d7a0ce6cc11aaa608b5ab7cb15cf047b622","unresolved":false,"context_lines":[{"line_number":117,"context_line":"        trunk \u003d cls(context, port_id\u003dport_id, tenant_id\u003dtenant_id, **fields)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        with db_api.autonested_transaction(context.session):"},{"line_number":120,"context_line":"            with trunk._check_create():"},{"line_number":121,"context_line":"                trunk.create()"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        return trunk"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_670a5fd4","line":120,"in_reply_to":"dab17558_75096e95","updated":"2016-05-07 00:21:17.000000000","message":"It had the opposite effect to my eyes as now I am gonna have to go all the way back to the fish for _check_create() when the method where it\u0027s used is only 5 lines of code :)","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":117,"context_line":"        trunk \u003d cls(context, port_id\u003dport_id, tenant_id\u003dtenant_id, **fields)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        with db_api.autonested_transaction(context.session):"},{"line_number":120,"context_line":"            with trunk._check_create():"},{"line_number":121,"context_line":"                trunk.create()"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        return trunk"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_a2114c06","line":120,"in_reply_to":"dab17558_eeb1da90","updated":"2016-05-11 06:59:21.000000000","message":"fixed","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":131,"context_line":"                try:"},{"line_number":132,"context_line":"                    self.detach_sub_port(sub_port_id)"},{"line_number":133,"context_line":"                except t_exc.SubPortNotFound:"},{"line_number":134,"context_line":"                    # sub-port was detached concurrently"},{"line_number":135,"context_line":"                    pass"},{"line_number":136,"context_line":"            self._sub_ports_remove(sub_port_ids)"},{"line_number":137,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_652b8531","line":134,"updated":"2016-05-06 18:25:57.000000000","message":"hang on! We need to make sure rules [1] are adopted. I don\u0027t think this is trivial as you make it, because without [1] we could simply rely on the DBMS\u0027s cascade policy.\n\n[1] https://github.com/openstack/neutron/blob/master/neutron/services/trunk/models.py#L67","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":131,"context_line":"                try:"},{"line_number":132,"context_line":"                    self.detach_sub_port(sub_port_id)"},{"line_number":133,"context_line":"                except t_exc.SubPortNotFound:"},{"line_number":134,"context_line":"                    # sub-port was detached concurrently"},{"line_number":135,"context_line":"                    pass"},{"line_number":136,"context_line":"            self._sub_ports_remove(sub_port_ids)"},{"line_number":137,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_22d6fcf0","line":134,"in_reply_to":"dab17558_47ae3b86","updated":"2016-05-11 06:59:21.000000000","message":"Please, see updated trunk/models.py with added SA event hooks (so, all constraints are in one place now, including (2) and (3), thanks for your suggestion). Had to remove the SA port relationship though, so that SA cascades do not interfere with our rules (2) and (3). Don\u0027t think we need them for OVO anyway.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":131,"context_line":"                try:"},{"line_number":132,"context_line":"                    self.detach_sub_port(sub_port_id)"},{"line_number":133,"context_line":"                except t_exc.SubPortNotFound:"},{"line_number":134,"context_line":"                    # sub-port was detached concurrently"},{"line_number":135,"context_line":"                    pass"},{"line_number":136,"context_line":"            self._sub_ports_remove(sub_port_ids)"},{"line_number":137,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_75a04ef5","line":134,"in_reply_to":"dab17558_652b8531","updated":"2016-05-06 23:19:39.000000000","message":"Rules (1) and (4) are already ensured and this check is not related to them. Rules (2) and (3) have to be addressed in the code related to the normal ports and I intend to do so under separate review (this one is already too large, as I\u0027ve been told).\n\nThe primary reason why this try-except block is here is that in its current state the VO\u0027s may easily become out of sync with the DB (and the other VO\u0027s copies). Consider that there are 2 concurrent requests. One deletes the trunk and the other detaches a subport. Both requests are valid, but with the right (or wrong) timing and without this check the \u0027delete\u0027 would fail with \u0027ObjectNotFound\u0027 while it really don\u0027t have to. This is covered in \u0027test__delete_trunk__concurrent\u0027 test case.\n\nThen there\u0027s another case when port is attached instead - it would get deleted due to the cascade constraint during self.delete, but not by the self.detach_sub_port. And there\u0027s literally nothing I can do with it right now.\n\nThe safest approach would be to lock the Trunk DB object (e.g. select-for-update) whenever subports are added/removed or the Trunk itself is being removed. It would make things rather simple at the price of serialising trunk mutations.\n\nThe other one would be to add \u0027subport-hash\u0027 to the Trunk DB model and implement proper CAS.\n\nRight now it is effectively a lock-free model but without the special handling and with lots of potential mines laying around. I only covered some (see test__*concurrent) cases just because the \u0027delete\u0027 was worth it (it does not really matter if detach_sub_port would fail due to delete, but not the other way).\n\nAnyway, as I see it, it is kind of fundamental \u0027feature\u0027 of VO\u0027s and out of the scope of this review.\n\nSo, it is probably worth discussing with others (weekly irc or maillist?) to form some common approach that will be used by all Neutron (container-like) VO\u0027s. Or maybe I\u0027m missing some point? What would you recommend?","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"a5aa0d7a0ce6cc11aaa608b5ab7cb15cf047b622","unresolved":false,"context_lines":[{"line_number":131,"context_line":"                try:"},{"line_number":132,"context_line":"                    self.detach_sub_port(sub_port_id)"},{"line_number":133,"context_line":"                except t_exc.SubPortNotFound:"},{"line_number":134,"context_line":"                    # sub-port was detached concurrently"},{"line_number":135,"context_line":"                    pass"},{"line_number":136,"context_line":"            self._sub_ports_remove(sub_port_ids)"},{"line_number":137,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_47ae3b86","line":134,"in_reply_to":"dab17558_75a04ef5","updated":"2016-05-07 00:21:17.000000000","message":"I am not suggesting to introduce locking or not dealing with interleaving operations on the trunk, all I am suggesting is that we should have a consolidated place where we embed these business rules so that\u0027s easy to maintain/debug them over time. Bear in mind that some operations can be made directly on the underlying resources (ie. the ports themselves via port API operations). We may need to employ callbacks to prevent/react to these operations.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":137,"context_line":""},{"line_number":138,"context_line":"            super(Trunk, self).delete()"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    def _create_sub_port(self, port_id, segmentation_type, segmentation_id):"},{"line_number":141,"context_line":"        # REVISIT(ivc): allow segmentation_id to be None (i.e. auto-generated)"},{"line_number":142,"context_line":"        sub_port \u003d SubPort(self._context, port_id\u003dport_id, trunk_id\u003dself.id,"},{"line_number":143,"context_line":"                           segmentation_type\u003dsegmentation_type,"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_a5928d55","line":140,"updated":"2016-05-06 18:25:57.000000000","message":"I appreciate the desire of developing isolated units and keep methods small and easy to test, but you\u0027re pushing this a little too far IMO. There are too many levels of indirections and I believe that some of these private methods may roll into their public counterparts. If we need reuse we can revise later.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":137,"context_line":""},{"line_number":138,"context_line":"            super(Trunk, self).delete()"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    def _create_sub_port(self, port_id, segmentation_type, segmentation_id):"},{"line_number":141,"context_line":"        # REVISIT(ivc): allow segmentation_id to be None (i.e. auto-generated)"},{"line_number":142,"context_line":"        sub_port \u003d SubPort(self._context, port_id\u003dport_id, trunk_id\u003dself.id,"},{"line_number":143,"context_line":"                           segmentation_type\u003dsegmentation_type,"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_627f64e0","line":140,"in_reply_to":"dab17558_8e2ace2a","updated":"2016-05-11 06:59:21.000000000","message":"@Rossella, the point is to have helper methods to create subports, so that the caller of these methods does not have to care about SubPort constructor/create \u0026 transaction management.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":137,"context_line":""},{"line_number":138,"context_line":"            super(Trunk, self).delete()"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    def _create_sub_port(self, port_id, segmentation_type, segmentation_id):"},{"line_number":141,"context_line":"        # REVISIT(ivc): allow segmentation_id to be None (i.e. auto-generated)"},{"line_number":142,"context_line":"        sub_port \u003d SubPort(self._context, port_id\u003dport_id, trunk_id\u003dself.id,"},{"line_number":143,"context_line":"                           segmentation_type\u003dsegmentation_type,"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_42f2209b","line":140,"in_reply_to":"dab17558_a5928d55","updated":"2016-05-11 06:59:21.000000000","message":"@Armando, fixed","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":137,"context_line":""},{"line_number":138,"context_line":"            super(Trunk, self).delete()"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    def _create_sub_port(self, port_id, segmentation_type, segmentation_id):"},{"line_number":141,"context_line":"        # REVISIT(ivc): allow segmentation_id to be None (i.e. auto-generated)"},{"line_number":142,"context_line":"        sub_port \u003d SubPort(self._context, port_id\u003dport_id, trunk_id\u003dself.id,"},{"line_number":143,"context_line":"                           segmentation_type\u003dsegmentation_type,"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_b5463621","line":140,"in_reply_to":"dab17558_a5928d55","updated":"2016-05-06 23:19:39.000000000","message":"It is reused by both attach_port and attach_ports. Actually, I was thinking about removing the attach/detach_port and just leaving the *_ports variants and merging the _create/delete_sub_port with them. The plugin part most likely would not need it anyway. But I was a bit lazy to rewrite tests for that.\n\nThanks, I\u0027ll fix that in next PS.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":137,"context_line":""},{"line_number":138,"context_line":"            super(Trunk, self).delete()"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    def _create_sub_port(self, port_id, segmentation_type, segmentation_id):"},{"line_number":141,"context_line":"        # REVISIT(ivc): allow segmentation_id to be None (i.e. auto-generated)"},{"line_number":142,"context_line":"        sub_port \u003d SubPort(self._context, port_id\u003dport_id, trunk_id\u003dself.id,"},{"line_number":143,"context_line":"                           segmentation_type\u003dsegmentation_type,"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_8e2ace2a","line":140,"in_reply_to":"dab17558_b5463621","updated":"2016-05-09 16:47:21.000000000","message":"I agree with Armando, I will take a look at next patch then. Another point is that the creation of the subport belongs to the SubPort class not to the Trunk. What you want is to override the create method in SubPort.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":138,"context_line":"            super(Trunk, self).delete()"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    def _create_sub_port(self, port_id, segmentation_type, segmentation_id):"},{"line_number":141,"context_line":"        # REVISIT(ivc): allow segmentation_id to be None (i.e. auto-generated)"},{"line_number":142,"context_line":"        sub_port \u003d SubPort(self._context, port_id\u003dport_id, trunk_id\u003dself.id,"},{"line_number":143,"context_line":"                           segmentation_type\u003dsegmentation_type,"},{"line_number":144,"context_line":"                           segmentation_id\u003dsegmentation_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2594dd35","line":141,"updated":"2016-05-06 18:25:57.000000000","message":"I think we\u0027d want the segmentation id always specified here and make the caller aware of any policy decision (whether it should be generated or populated from the port\u0027s network).","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":138,"context_line":"            super(Trunk, self).delete()"},{"line_number":139,"context_line":""},{"line_number":140,"context_line":"    def _create_sub_port(self, port_id, segmentation_type, segmentation_id):"},{"line_number":141,"context_line":"        # REVISIT(ivc): allow segmentation_id to be None (i.e. auto-generated)"},{"line_number":142,"context_line":"        sub_port \u003d SubPort(self._context, port_id\u003dport_id, trunk_id\u003dself.id,"},{"line_number":143,"context_line":"                           segmentation_type\u003dsegmentation_type,"},{"line_number":144,"context_line":"                           segmentation_id\u003dsegmentation_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_d5581afd","line":141,"in_reply_to":"dab17558_2594dd35","updated":"2016-05-06 23:19:39.000000000","message":"Ok. Good point.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"734376bed0731caa7a416423cdb30b58d51c8982","unresolved":false,"context_lines":[{"line_number":154,"context_line":"        if not sub_port:"},{"line_number":155,"context_line":"            raise t_exc.SubPortNotFound(port_id\u003dport_id, trunk_id\u003dself.id)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        sub_port.delete()"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _sub_ports_extend(self, sub_ports):"},{"line_number":160,"context_line":"        field \u003d \u0027sub_ports\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_a5a0cd91","line":157,"updated":"2016-05-06 18:25:57.000000000","message":"can\u0027t we issue a straight delete? If not never mind.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"a5aa0d7a0ce6cc11aaa608b5ab7cb15cf047b622","unresolved":false,"context_lines":[{"line_number":154,"context_line":"        if not sub_port:"},{"line_number":155,"context_line":"            raise t_exc.SubPortNotFound(port_id\u003dport_id, trunk_id\u003dself.id)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        sub_port.delete()"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _sub_ports_extend(self, sub_ports):"},{"line_number":160,"context_line":"        field \u003d \u0027sub_ports\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_471bdb1b","line":157,"in_reply_to":"dab17558_2f6033ce","updated":"2016-05-07 00:21:17.000000000","message":"rather than getting and deleting, I wonder if if we can issue a delete query instead.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":154,"context_line":"        if not sub_port:"},{"line_number":155,"context_line":"            raise t_exc.SubPortNotFound(port_id\u003dport_id, trunk_id\u003dself.id)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        sub_port.delete()"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _sub_ports_extend(self, sub_ports):"},{"line_number":160,"context_line":"        field \u003d \u0027sub_ports\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_cd08012b","line":157,"in_reply_to":"dab17558_471bdb1b","updated":"2016-05-11 06:59:21.000000000","message":"@Armando, @Artur, actually, we can if we use obj_db_api as delete itself does. It would be a bad idea to do so for external OVOs, but should be fine for SubPort. So... fixed! :)","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"b580046329196995606a7977c71651ad1fa479a7","unresolved":false,"context_lines":[{"line_number":154,"context_line":"        if not sub_port:"},{"line_number":155,"context_line":"            raise t_exc.SubPortNotFound(port_id\u003dport_id, trunk_id\u003dself.id)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        sub_port.delete()"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _sub_ports_extend(self, sub_ports):"},{"line_number":160,"context_line":"        field \u003d \u0027sub_ports\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_ef9c425f","line":157,"in_reply_to":"dab17558_471bdb1b","updated":"2016-05-09 12:40:52.000000000","message":"When using objects you need first to get the object in order to delete it...","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"7c7eb7782429c6ba0d1a026512e334ffb193567e","unresolved":false,"context_lines":[{"line_number":154,"context_line":"        if not sub_port:"},{"line_number":155,"context_line":"            raise t_exc.SubPortNotFound(port_id\u003dport_id, trunk_id\u003dself.id)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        sub_port.delete()"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _sub_ports_extend(self, sub_ports):"},{"line_number":160,"context_line":"        field \u003d \u0027sub_ports\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2f6033ce","line":157,"in_reply_to":"dab17558_a5a0cd91","updated":"2016-05-06 23:19:39.000000000","message":"What do you mean by \u0027straight delete\u0027?","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":154,"context_line":"        if not sub_port:"},{"line_number":155,"context_line":"            raise t_exc.SubPortNotFound(port_id\u003dport_id, trunk_id\u003dself.id)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        sub_port.delete()"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _sub_ports_extend(self, sub_ports):"},{"line_number":160,"context_line":"        field \u003d \u0027sub_ports\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_f1425f3d","line":157,"in_reply_to":"dab17558_ef9c425f","updated":"2016-05-09 16:47:21.000000000","message":"we should override the delete method in SubPort","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":154,"context_line":"        if not sub_port:"},{"line_number":155,"context_line":"            raise t_exc.SubPortNotFound(port_id\u003dport_id, trunk_id\u003dself.id)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        sub_port.delete()"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _sub_ports_extend(self, sub_ports):"},{"line_number":160,"context_line":"        field \u003d \u0027sub_ports\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_0d17a948","line":157,"in_reply_to":"dab17558_f1425f3d","updated":"2016-05-11 06:59:21.000000000","message":"@Rossella, please, see my comment on L140","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":206,"context_line":"                                             segmentation_type\u003dseg_type,"},{"line_number":207,"context_line":"                                             segmentation_id\u003dseg_id)"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":"        self._sub_ports_extend([sub_port])"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    def attach_sub_ports(self, ports):"},{"line_number":212,"context_line":"        \"\"\"Attach sub-ports."}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_3164e7e2","line":209,"updated":"2016-05-09 16:47:21.000000000","message":"I think this submethod is not really needed, too many levels of indirection. Line 235 should be in the same transaction as the subport creation","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":206,"context_line":"                                             segmentation_type\u003dseg_type,"},{"line_number":207,"context_line":"                                             segmentation_id\u003dseg_id)"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":"        self._sub_ports_extend([sub_port])"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    def attach_sub_ports(self, ports):"},{"line_number":212,"context_line":"        \"\"\"Attach sub-ports."}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_8d08b9db","line":209,"in_reply_to":"dab17558_3164e7e2","updated":"2016-05-11 06:59:21.000000000","message":"submethod fixed. L235 must not be in the same transaction, otherwise \u0027sub_ports\u0027 might become corrupted in case of transaction rollback.","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":250,"context_line":"            self._delete_sub_port(port_id)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        self._sub_ports_remove([port_id])"},{"line_number":253,"context_line":""},{"line_number":254,"context_line":"    def detach_sub_ports(self, port_ids):"},{"line_number":255,"context_line":"        \"\"\"Detach a sub-port."}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2e4be2ca","line":252,"updated":"2016-05-09 16:47:21.000000000","message":"ditto as line 209","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":250,"context_line":"            self._delete_sub_port(port_id)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        self._sub_ports_remove([port_id])"},{"line_number":253,"context_line":""},{"line_number":254,"context_line":"    def detach_sub_ports(self, port_ids):"},{"line_number":255,"context_line":"        \"\"\"Detach a sub-port."}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_edf7c5d0","line":252,"in_reply_to":"dab17558_2e4be2ca","updated":"2016-05-11 06:59:21.000000000","message":"ditto","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":252,"context_line":"        self._sub_ports_remove([port_id])"},{"line_number":253,"context_line":""},{"line_number":254,"context_line":"    def detach_sub_ports(self, port_ids):"},{"line_number":255,"context_line":"        \"\"\"Detach a sub-port."},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"        Batch version of :method:`detach_sub_port`. If any port fails to be"},{"line_number":258,"context_line":"        detached, the trunk is left unaffected."}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_4e2f46f6","line":255,"updated":"2016-05-09 16:47:21.000000000","message":"nit: plural","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4a67edc2cb545f4899425f8f93bb629206512e42","unresolved":false,"context_lines":[{"line_number":252,"context_line":"        self._sub_ports_remove([port_id])"},{"line_number":253,"context_line":""},{"line_number":254,"context_line":"    def detach_sub_ports(self, port_ids):"},{"line_number":255,"context_line":"        \"\"\"Detach a sub-port."},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"        Batch version of :method:`detach_sub_port`. If any port fails to be"},{"line_number":258,"context_line":"        detached, the trunk is left unaffected."}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_2dfeedad","line":255,"in_reply_to":"dab17558_4e2f46f6","updated":"2016-05-11 06:59:21.000000000","message":"fixed","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":6788,"name":"Rossella Sblendido","email":"rsblendido@suse.com","username":"rossella-o"},"change_message_id":"10971ab6d04f6a8a40c740cd7624e5ebbc59d2da","unresolved":false,"context_lines":[{"line_number":262,"context_line":"                                 the current trunk"},{"line_number":263,"context_line":"        \"\"\""},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":266,"context_line":"            for port_id in port_ids:"},{"line_number":267,"context_line":"                self._delete_sub_port(port_id)"},{"line_number":268,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"dab17558_4ecbc61e","line":265,"updated":"2016-05-09 16:47:21.000000000","message":"instead of repeating you could do:\n\nwith db_api.autonested_transaction(self._context.session):\n    for port_id in port_ids:\n        self.detach_sub_port(port_id)","commit_id":"6fe9295818748aaa454e65d548656413cad392a9"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f9c03f00c5b5dac72fa8cf03abdb7c06c740d5eb","unresolved":false,"context_lines":[{"line_number":112,"context_line":""},{"line_number":113,"context_line":"    fields \u003d {"},{"line_number":114,"context_line":"        \u0027id\u0027: obj_fields.UUIDField(),"},{"line_number":115,"context_line":"        \u0027tenant_id\u0027: obj_fields.UUIDField(),"},{"line_number":116,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"},{"line_number":117,"context_line":"        \u0027sub_ports\u0027: obj_fields.ListOfObjectsField(\u0027SubPort\u0027),"},{"line_number":118,"context_line":"    }"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_a8000b67","line":115,"range":{"start_line":115,"start_character":9,"end_line":115,"end_character":18},"updated":"2016-05-11 07:18:42.000000000","message":"It should be project_id, because of general tenant_id -\u003e project_id renaming effort [1].\n\n[1] https://review.openstack.org/#/c/257362/15/specs/newton/moving-to-keystone-v3.rst@32","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"07c78ec6cbcc4c5b5c7e4053c171d2ec29132c66","unresolved":false,"context_lines":[{"line_number":112,"context_line":""},{"line_number":113,"context_line":"    fields \u003d {"},{"line_number":114,"context_line":"        \u0027id\u0027: obj_fields.UUIDField(),"},{"line_number":115,"context_line":"        \u0027tenant_id\u0027: obj_fields.UUIDField(),"},{"line_number":116,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"},{"line_number":117,"context_line":"        \u0027sub_ports\u0027: obj_fields.ListOfObjectsField(\u0027SubPort\u0027),"},{"line_number":118,"context_line":"    }"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_e13932da","line":115,"range":{"start_line":115,"start_character":9,"end_line":115,"end_character":18},"in_reply_to":"dab17558_361c2277","updated":"2016-05-12 17:26:27.000000000","message":"though we can ask for opinions by the spec author.","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"c6ec853082150b1aecff37cc886721f76c6469a3","unresolved":false,"context_lines":[{"line_number":112,"context_line":""},{"line_number":113,"context_line":"    fields \u003d {"},{"line_number":114,"context_line":"        \u0027id\u0027: obj_fields.UUIDField(),"},{"line_number":115,"context_line":"        \u0027tenant_id\u0027: obj_fields.UUIDField(),"},{"line_number":116,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"},{"line_number":117,"context_line":"        \u0027sub_ports\u0027: obj_fields.ListOfObjectsField(\u0027SubPort\u0027),"},{"line_number":118,"context_line":"    }"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_64dc4775","line":115,"range":{"start_line":115,"start_character":9,"end_line":115,"end_character":18},"in_reply_to":"dab17558_53db141b","updated":"2016-05-12 11:19:43.000000000","message":"As it seems, the spec mentioned is not merged yet. And we do not have HasProject in place of HasTenant mixin. So I\u0027m leaving \u0027tenant_id\u0027 as it is for now unless the spec is merged and the proper mixin is provided before this change is approved.","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"8ade37f86d01bf1668d03a4bbe221fe34e996221","unresolved":false,"context_lines":[{"line_number":112,"context_line":""},{"line_number":113,"context_line":"    fields \u003d {"},{"line_number":114,"context_line":"        \u0027id\u0027: obj_fields.UUIDField(),"},{"line_number":115,"context_line":"        \u0027tenant_id\u0027: obj_fields.UUIDField(),"},{"line_number":116,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"},{"line_number":117,"context_line":"        \u0027sub_ports\u0027: obj_fields.ListOfObjectsField(\u0027SubPort\u0027),"},{"line_number":118,"context_line":"    }"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_361c2277","line":115,"range":{"start_line":115,"start_character":9,"end_line":115,"end_character":18},"in_reply_to":"dab17558_64dc4775","updated":"2016-05-12 17:25:51.000000000","message":"It\u0027s probably best to leave this as tenant_id for now.","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"07e7d2fc9f2147b14857c65353f4922b113deff3","unresolved":false,"context_lines":[{"line_number":112,"context_line":""},{"line_number":113,"context_line":"    fields \u003d {"},{"line_number":114,"context_line":"        \u0027id\u0027: obj_fields.UUIDField(),"},{"line_number":115,"context_line":"        \u0027tenant_id\u0027: obj_fields.UUIDField(),"},{"line_number":116,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"},{"line_number":117,"context_line":"        \u0027sub_ports\u0027: obj_fields.ListOfObjectsField(\u0027SubPort\u0027),"},{"line_number":118,"context_line":"    }"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_53db141b","line":115,"range":{"start_line":115,"start_character":9,"end_line":115,"end_character":18},"in_reply_to":"dab17558_a8000b67","updated":"2016-05-11 08:53:48.000000000","message":"ok. noted. will fix in next PS","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f9c03f00c5b5dac72fa8cf03abdb7c06c740d5eb","unresolved":false,"context_lines":[{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    def delete(self):"},{"line_number":125,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":126,"context_line":"            self.load_synthetic_db_fields()"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            sub_port_ids \u003d [p.port_id for p in self.sub_ports]"},{"line_number":129,"context_line":"            self.detach_sub_ports(sub_port_ids)"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_682c6399","line":126,"range":{"start_line":126,"start_character":12,"end_line":126,"end_character":43},"updated":"2016-05-11 07:18:42.000000000","message":"Do you need to load the synthetic fields in delete() method? Object itself should have those fields loaded...","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"07e7d2fc9f2147b14857c65353f4922b113deff3","unresolved":false,"context_lines":[{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    def delete(self):"},{"line_number":125,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":126,"context_line":"            self.load_synthetic_db_fields()"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            sub_port_ids \u003d [p.port_id for p in self.sub_ports]"},{"line_number":129,"context_line":"            self.detach_sub_ports(sub_port_ids)"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_ae37734b","line":126,"range":{"start_line":126,"start_character":12,"end_line":126,"end_character":43},"in_reply_to":"dab17558_682c6399","updated":"2016-05-11 08:53:48.000000000","message":"I was planing some updates to \u0027NeutronDbObject\u0027 that would require it (outside of this review). But you are right that as of now it is redundant. Will remove it in the next PS.","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f9c03f00c5b5dac72fa8cf03abdb7c06c740d5eb","unresolved":false,"context_lines":[{"line_number":125,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":126,"context_line":"            self.load_synthetic_db_fields()"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            sub_port_ids \u003d [p.port_id for p in self.sub_ports]"},{"line_number":129,"context_line":"            self.detach_sub_ports(sub_port_ids)"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"            setattr(self, \u0027sub_ports\u0027, [])"},{"line_number":132,"context_line":"            self.obj_reset_changes([\u0027sub_ports\u0027])"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_e8c21384","line":129,"range":{"start_line":128,"start_character":12,"end_line":129,"end_character":47},"updated":"2016-05-11 07:18:42.000000000","message":"Why can\u0027t you delete the sub_ports in here using the object, rather then using the db_api and sub_port_id?\nyou can go over here:\nfor sub_port in self.sub_ports:\n    sub_port.delete()","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"07e7d2fc9f2147b14857c65353f4922b113deff3","unresolved":false,"context_lines":[{"line_number":125,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":126,"context_line":"            self.load_synthetic_db_fields()"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            sub_port_ids \u003d [p.port_id for p in self.sub_ports]"},{"line_number":129,"context_line":"            self.detach_sub_ports(sub_port_ids)"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"            setattr(self, \u0027sub_ports\u0027, [])"},{"line_number":132,"context_line":"            self.obj_reset_changes([\u0027sub_ports\u0027])"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_4e77e701","line":129,"range":{"start_line":128,"start_character":12,"end_line":129,"end_character":47},"in_reply_to":"dab17558_e8c21384","updated":"2016-05-11 08:53:48.000000000","message":"Let\u0027s see what others would say about obj_db_api.delete_object in detach_sub_ports. If it stays, then I see no reason not to use detach_sub_ports here (as it would also handle the \u0027sub_ports\u0027 field).","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"07e7d2fc9f2147b14857c65353f4922b113deff3","unresolved":false,"context_lines":[{"line_number":128,"context_line":"            sub_port_ids \u003d [p.port_id for p in self.sub_ports]"},{"line_number":129,"context_line":"            self.detach_sub_ports(sub_port_ids)"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"            setattr(self, \u0027sub_ports\u0027, [])"},{"line_number":132,"context_line":"            self.obj_reset_changes([\u0027sub_ports\u0027])"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"            super(Trunk, self).delete()"},{"line_number":135,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_ae5c937d","line":132,"range":{"start_line":131,"start_character":0,"end_line":132,"end_character":49},"updated":"2016-05-11 08:53:48.000000000","message":"this is redundant and to be removed","commit_id":"d2ed076ae487690e2c86b0f82ca3741158f3c51b"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"766dae2db67d1c5baa881a2697417c607b01cd20","unresolved":false,"context_lines":[{"line_number":112,"context_line":""},{"line_number":113,"context_line":"    fields \u003d {"},{"line_number":114,"context_line":"        \u0027id\u0027: obj_fields.UUIDField(),"},{"line_number":115,"context_line":"        \u0027tenant_id\u0027: obj_fields.UUIDField(),"},{"line_number":116,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"},{"line_number":117,"context_line":"        \u0027sub_ports\u0027: obj_fields.ListOfObjectsField(\u0027SubPort\u0027),"},{"line_number":118,"context_line":"    }"}],"source_content_type":"text/x-python","patch_set":10,"id":"dab17558_4e08392e","line":115,"range":{"start_line":115,"start_character":9,"end_line":115,"end_character":18},"updated":"2016-05-12 13:12:47.000000000","message":"If you want to expose the project_id instead of tenant_id, there is already implemented solution in OVO base class: modify_fields(to|from)_db() method, which can automatically translate the project \u003c-\u003e tenant when fetching and saving the data in DB. For reference, you can take a look at my patch [1]. \n\nThe question here is do you want to expose the \u0027project\u0027 interface in OVO from the beginning, or do you want to rework the OVO after the project field is introduced (it would require bumping the version of OVO, support mixed version running in the same time etc.)\n\nI would go for project_id from the beginning, but let\u0027s see what is others opinion.\n\n[1] https://review.openstack.org/#/c/264273/24/neutron/objects/subnet.py@151","commit_id":"54799632213b600aa6c281846e1f9b735d714d68"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from neutron.services.trunk import models"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"def get_trunk(context, port_id):"},{"line_number":28,"context_line":"    \"\"\"Get :class:`Trunk` object for the specified `port_id`."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    Loads the :class:`Trunk` object corresponding to the specified trunk"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_31ba2724","line":27,"updated":"2016-05-13 22:16:13.000000000","message":"shouldn\u0027t this be get_trunk_by_parent?\n\nWe could have one method that looks up by trunk id, no?","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from neutron.services.trunk import models"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"def get_trunk(context, port_id):"},{"line_number":28,"context_line":"    \"\"\"Get :class:`Trunk` object for the specified `port_id`."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    Loads the :class:`Trunk` object corresponding to the specified trunk"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_bd3116ef","line":27,"in_reply_to":"dab17558_16230306","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from neutron.services.trunk import models"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"def get_trunk(context, port_id):"},{"line_number":28,"context_line":"    \"\"\"Get :class:`Trunk` object for the specified `port_id`."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    Loads the :class:`Trunk` object corresponding to the specified trunk"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_82b61fda","line":27,"in_reply_to":"dab17558_31ba2724","updated":"2016-05-14 01:28:21.000000000","message":"What do you say if I make it \u0027get_trunk(context, port_id\u003dNone, trunk_id\u003dNone)\u0027? That would solve both problems and would not require duplicating the same code.\n\nActually I was also thinking about extending NeutronDbObject with unique_keys in addition to primary keys and making get_object accept those without throwing an error. That would be a pretty simple change I think and would make things a bit more consistent/sane. But I\u0027d like to keep this helper anyway for proper exceptions.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from neutron.services.trunk import models"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"def get_trunk(context, port_id):"},{"line_number":28,"context_line":"    \"\"\"Get :class:`Trunk` object for the specified `port_id`."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    Loads the :class:`Trunk` object corresponding to the specified trunk"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_16230306","line":27,"in_reply_to":"dab17558_82b61fda","updated":"2016-05-18 01:07:38.000000000","message":"I am not sure having a single method that handles both cases is getting us much especially if we don\u0027t end up using one way to look up trunks. I\u0027d suggest to start simple.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    :returns: :class:`Trunk` -- trunk object"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    :raises TrunkPortNotFound: if the specified port is not a trunk port"},{"line_number":39,"context_line":"    \"\"\""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    trunks \u003d Trunk.get_objects(context, port_id\u003dport_id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_51ec0b29","line":38,"updated":"2016-05-13 22:16:13.000000000","message":"I think this should be TrunkNotFound: ie the trunk cannot be found, but the port might as well exist","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    :returns: :class:`Trunk` -- trunk object"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    :raises TrunkPortNotFound: if the specified port is not a trunk port"},{"line_number":39,"context_line":"    \"\"\""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    trunks \u003d Trunk.get_objects(context, port_id\u003dport_id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_c244a775","line":38,"in_reply_to":"dab17558_51ec0b29","updated":"2016-05-14 01:28:21.000000000","message":"Well, the problem here is that we have two keys for trunk lookup: port_id and trunk_id (tbh, I\u0027m not sure if we need trunk_id at all). So the intent was to have different exception messages and keep them all here. But TrunkPortNotFound is a subclass of TrunkNotFound, so the caller can check just for the latter.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    :returns: :class:`Trunk` -- trunk object"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    :raises TrunkPortNotFound: if the specified port is not a trunk port"},{"line_number":39,"context_line":"    \"\"\""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    trunks \u003d Trunk.get_objects(context, port_id\u003dport_id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_d6833bc9","line":38,"in_reply_to":"dab17558_c244a775","updated":"2016-05-18 01:07:38.000000000","message":"Trunk is a top-level resource, and thus you have/need an identifier.\n\nThe objective of this method is to look up a trunk by port id: if the lookup fails, then the most sensible error to return is TrunkNotFound. Keep it simple.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":"    :returns: :class:`Trunk` -- trunk object"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    :raises TrunkPortNotFound: if the specified port is not a trunk port"},{"line_number":39,"context_line":"    \"\"\""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    trunks \u003d Trunk.get_objects(context, port_id\u003dport_id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_5d6e2aca","line":38,"in_reply_to":"dab17558_d6833bc9","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":61,"context_line":""},{"line_number":62,"context_line":"    :returns: :class:`Trunk` -- new trunk object"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    :raises PortNotFound: if the specified port does not exist"},{"line_number":65,"context_line":"    :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":66,"context_line":"                            trunk port"},{"line_number":67,"context_line":"    :raises SubPortInUse: if the specified port is a sub-port of another"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_baaef195","line":64,"updated":"2016-05-13 22:16:13.000000000","message":"which one? Could be both port_id or any within sub_ports","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":61,"context_line":""},{"line_number":62,"context_line":"    :returns: :class:`Trunk` -- new trunk object"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    :raises PortNotFound: if the specified port does not exist"},{"line_number":65,"context_line":"    :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":66,"context_line":"                            trunk port"},{"line_number":67,"context_line":"    :raises SubPortInUse: if the specified port is a sub-port of another"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_bdb6363b","line":64,"in_reply_to":"dab17558_a215234a","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":61,"context_line":""},{"line_number":62,"context_line":"    :returns: :class:`Trunk` -- new trunk object"},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"    :raises PortNotFound: if the specified port does not exist"},{"line_number":65,"context_line":"    :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":66,"context_line":"                            trunk port"},{"line_number":67,"context_line":"    :raises SubPortInUse: if the specified port is a sub-port of another"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_a215234a","line":64,"in_reply_to":"dab17558_baaef195","updated":"2016-05-14 01:28:21.000000000","message":"trunk one. will fix this \u0026 L70","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":67,"context_line":"    :raises SubPortInUse: if the specified port is a sub-port of another"},{"line_number":68,"context_line":"                          trunk"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    :raises PortNotFound: if the specified sub-port does not exist"},{"line_number":71,"context_line":"    :raises TrunkPortInUse: if the specified sub-port is already used as a"},{"line_number":72,"context_line":"                            trunk port"},{"line_number":73,"context_line":"    :raises SubPortInUse: if the specified sub-port is already a sub-port"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_da82d5ff","line":70,"updated":"2016-05-13 22:16:13.000000000","message":"I\u0027d collapse the two see line 64","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":67,"context_line":"    :raises SubPortInUse: if the specified port is a sub-port of another"},{"line_number":68,"context_line":"                          trunk"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"    :raises PortNotFound: if the specified sub-port does not exist"},{"line_number":71,"context_line":"    :raises TrunkPortInUse: if the specified sub-port is already used as a"},{"line_number":72,"context_line":"                            trunk port"},{"line_number":73,"context_line":"    :raises SubPortInUse: if the specified sub-port is already a sub-port"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_e2072b1b","line":70,"in_reply_to":"dab17558_da82d5ff","updated":"2016-05-14 01:28:21.000000000","message":"ok","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    :raises PortNotFound: if the specified sub-port does not exist"},{"line_number":71,"context_line":"    :raises TrunkPortInUse: if the specified sub-port is already used as a"},{"line_number":72,"context_line":"                            trunk port"},{"line_number":73,"context_line":"    :raises SubPortInUse: if the specified sub-port is already a sub-port"},{"line_number":74,"context_line":"    :raises InvalidSegmentationType: if the specified sub-port"},{"line_number":75,"context_line":"                                     `segmentation_type` is not supported"},{"line_number":76,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_7a7c89eb","line":73,"updated":"2016-05-13 22:16:13.000000000","message":"I find duplicating the same exception in the docstring confusing; also I would suggest we minimize the number of error conditions if possible. I think we only need the following:\n\nPortNotFound: raised if a port to create a trunk is not available.\n\nThe uuid returned with the exception is enough for the caller to understand whether it\u0027s a parent or sub port. And the failure mode is the same, the trunk can\u0027t be created for lack of resources.\n\nInvalidPortForTrunk: raised if a port cannot be used in a trunk.\n\nthe uuid returned with the exception is enough for the caller to understand which port is in question. I would also add a reason to the exception that explain why the port is invalid (e.g. used by another trunk or because it\u0027s a special port, dhcp, router, it\u0027s already bound etc).","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    :raises PortNotFound: if the specified sub-port does not exist"},{"line_number":71,"context_line":"    :raises TrunkPortInUse: if the specified sub-port is already used as a"},{"line_number":72,"context_line":"                            trunk port"},{"line_number":73,"context_line":"    :raises SubPortInUse: if the specified sub-port is already a sub-port"},{"line_number":74,"context_line":"    :raises InvalidSegmentationType: if the specified sub-port"},{"line_number":75,"context_line":"                                     `segmentation_type` is not supported"},{"line_number":76,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_bd8d967d","line":73,"in_reply_to":"dab17558_1d5e1212","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":70,"context_line":"    :raises PortNotFound: if the specified sub-port does not exist"},{"line_number":71,"context_line":"    :raises TrunkPortInUse: if the specified sub-port is already used as a"},{"line_number":72,"context_line":"                            trunk port"},{"line_number":73,"context_line":"    :raises SubPortInUse: if the specified sub-port is already a sub-port"},{"line_number":74,"context_line":"    :raises InvalidSegmentationType: if the specified sub-port"},{"line_number":75,"context_line":"                                     `segmentation_type` is not supported"},{"line_number":76,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_1d5e1212","line":73,"in_reply_to":"dab17558_7a7c89eb","updated":"2016-05-14 01:28:21.000000000","message":"ok","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    :raises TrunkPortInUse: if the specified sub-port is already used as a"},{"line_number":72,"context_line":"                            trunk port"},{"line_number":73,"context_line":"    :raises SubPortInUse: if the specified sub-port is already a sub-port"},{"line_number":74,"context_line":"    :raises InvalidSegmentationType: if the specified sub-port"},{"line_number":75,"context_line":"                                     `segmentation_type` is not supported"},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_5a826595","line":74,"updated":"2016-05-13 22:16:13.000000000","message":"If we raise this we should raise an Invalid Segmentation ID too. Perhaps we can have an InvalidSegmentionInfo that contains a reason, either the type or the is is wrong or even if the id is non unique within the trunk","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    :raises TrunkPortInUse: if the specified sub-port is already used as a"},{"line_number":72,"context_line":"                            trunk port"},{"line_number":73,"context_line":"    :raises SubPortInUse: if the specified sub-port is already a sub-port"},{"line_number":74,"context_line":"    :raises InvalidSegmentationType: if the specified sub-port"},{"line_number":75,"context_line":"                                     `segmentation_type` is not supported"},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_5d29ba88","line":74,"in_reply_to":"dab17558_5a826595","updated":"2016-05-14 01:28:21.000000000","message":"ok","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    :raises TrunkPortInUse: if the specified sub-port is already used as a"},{"line_number":72,"context_line":"                            trunk port"},{"line_number":73,"context_line":"    :raises SubPortInUse: if the specified sub-port is already a sub-port"},{"line_number":74,"context_line":"    :raises InvalidSegmentationType: if the specified sub-port"},{"line_number":75,"context_line":"                                     `segmentation_type` is not supported"},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_fdd43e48","line":74,"in_reply_to":"dab17558_5d29ba88","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    db_model \u003d models.SubPort"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"    primary_keys \u003d [\u0027port_id\u0027]"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    fields \u003d {"},{"line_number":99,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_ce3a1634","line":96,"updated":"2016-05-13 22:16:13.000000000","message":"primary keys for subport are port id and trunk id, is this an oversight?","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    db_model \u003d models.SubPort"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"    primary_keys \u003d [\u0027port_id\u0027]"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    fields \u003d {"},{"line_number":99,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_fd627eac","line":96,"in_reply_to":"dab17558_5d3a5a99","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":93,"context_line":""},{"line_number":94,"context_line":"    db_model \u003d models.SubPort"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"    primary_keys \u003d [\u0027port_id\u0027]"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    fields \u003d {"},{"line_number":99,"context_line":"        \u0027port_id\u0027: obj_fields.UUIDField(),"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_5d3a5a99","line":96,"in_reply_to":"dab17558_ce3a1634","updated":"2016-05-14 01:28:21.000000000","message":"Good catch! They are in the DB, but port_id is unique/non-nullable and qualifies as PK. Not really an oversight, but the legacy of PS1/2 that I totally forgot about as I wanted to  allow SubPort.get_object with only port_id. Will fix.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    synthetic_fields \u003d [\u0027sub_ports\u0027]"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def delete(self):"},{"line_number":127,"context_line":"        with db_api.autonested_transaction(self._context.session):"},{"line_number":128,"context_line":"            if self.obj_attr_is_set(\u0027sub_ports\u0027):"},{"line_number":129,"context_line":"                sub_port_ids \u003d [p.port_id for p in self.sub_ports]"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_9d0c1244","line":126,"range":{"start_line":126,"start_character":8,"end_line":126,"end_character":14},"updated":"2016-05-18 12:45:46.000000000","message":"Covered by db cascade delete. Service plugin will take care of properly detaching actual sub-ports before calling Trunk.delete.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":140,"context_line":"                          \u0027segmentation_type\u0027, \u0027segmentation_id\u0027"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"        :raises PortNotFound: if the specified port does not exist"},{"line_number":143,"context_line":"        :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":144,"context_line":"                                trunk port"},{"line_number":145,"context_line":"        :raises SubPortInUse: if the specified port is already a sub-port"},{"line_number":146,"context_line":"        :raises TrunkSegmentInUse: if the specified segment is already used"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_4ecaa61b","line":143,"updated":"2016-05-13 22:16:13.000000000","message":"how could this be raised if I am attaching subports?","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":140,"context_line":"                          \u0027segmentation_type\u0027, \u0027segmentation_id\u0027"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"        :raises PortNotFound: if the specified port does not exist"},{"line_number":143,"context_line":"        :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":144,"context_line":"                                trunk port"},{"line_number":145,"context_line":"        :raises SubPortInUse: if the specified port is already a sub-port"},{"line_number":146,"context_line":"        :raises TrunkSegmentInUse: if the specified segment is already used"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_dd464a20","line":143,"in_reply_to":"dab17558_4ecaa61b","updated":"2016-05-14 01:28:21.000000000","message":"That\u0027s for nested trunk constraint.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":140,"context_line":"                          \u0027segmentation_type\u0027, \u0027segmentation_id\u0027"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"        :raises PortNotFound: if the specified port does not exist"},{"line_number":143,"context_line":"        :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":144,"context_line":"                                trunk port"},{"line_number":145,"context_line":"        :raises SubPortInUse: if the specified port is already a sub-port"},{"line_number":146,"context_line":"        :raises TrunkSegmentInUse: if the specified segment is already used"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_9813805c","line":143,"in_reply_to":"dab17558_dd464a20","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        :raises PortNotFound: if the specified port does not exist"},{"line_number":143,"context_line":"        :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":144,"context_line":"                                trunk port"},{"line_number":145,"context_line":"        :raises SubPortInUse: if the specified port is already a sub-port"},{"line_number":146,"context_line":"        :raises TrunkSegmentInUse: if the specified segment is already used"},{"line_number":147,"context_line":"                                   for this trunk"},{"line_number":148,"context_line":"        :raises InvalidSegmentationType: if the specified"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_4e63c641","line":145,"updated":"2016-05-13 22:16:13.000000000","message":"same as suggested above. Perhaps it\u0027s wiser to collapse these exceptions.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        :raises PortNotFound: if the specified port does not exist"},{"line_number":143,"context_line":"        :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":144,"context_line":"                                trunk port"},{"line_number":145,"context_line":"        :raises SubPortInUse: if the specified port is already a sub-port"},{"line_number":146,"context_line":"        :raises TrunkSegmentInUse: if the specified segment is already used"},{"line_number":147,"context_line":"                                   for this trunk"},{"line_number":148,"context_line":"        :raises InvalidSegmentationType: if the specified"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_5820f89b","line":145,"in_reply_to":"dab17558_1d51f2d8","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        :raises PortNotFound: if the specified port does not exist"},{"line_number":143,"context_line":"        :raises TrunkPortInUse: if the specified port is already used as a"},{"line_number":144,"context_line":"                                trunk port"},{"line_number":145,"context_line":"        :raises SubPortInUse: if the specified port is already a sub-port"},{"line_number":146,"context_line":"        :raises TrunkSegmentInUse: if the specified segment is already used"},{"line_number":147,"context_line":"                                   for this trunk"},{"line_number":148,"context_line":"        :raises InvalidSegmentationType: if the specified"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_1d51f2d8","line":145,"in_reply_to":"dab17558_4e63c641","updated":"2016-05-14 01:28:21.000000000","message":"agreed.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":149,"context_line":"                                         `segmentation_type` is not supported"},{"line_number":150,"context_line":"        \"\"\""},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"        def _create_sub_port(port_dict):"},{"line_number":153,"context_line":"            sub_port \u003d SubPort(self._context, trunk_id\u003dself.id, **port_dict)"},{"line_number":154,"context_line":"            sub_port.create()"},{"line_number":155,"context_line":"            return sub_port"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_8eebee77","line":152,"updated":"2016-05-13 22:16:13.000000000","message":"nested methods are tricky to test, I\u0027d suggest to make it a proper method.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":149,"context_line":"                                         `segmentation_type` is not supported"},{"line_number":150,"context_line":"        \"\"\""},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"        def _create_sub_port(port_dict):"},{"line_number":153,"context_line":"            sub_port \u003d SubPort(self._context, trunk_id\u003dself.id, **port_dict)"},{"line_number":154,"context_line":"            sub_port.create()"},{"line_number":155,"context_line":"            return sub_port"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_9d29623a","line":152,"in_reply_to":"dab17558_8eebee77","updated":"2016-05-14 01:28:21.000000000","message":"It was a proper method before, but I was told that it added too much indirection. It is here just for the list comprehension on L158.\n\nI\u0027ll make it into a for loop, otherwise someone could post another \u0027indirection\u0027 comment :/","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":149,"context_line":"                                         `segmentation_type` is not supported"},{"line_number":150,"context_line":"        \"\"\""},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"        def _create_sub_port(port_dict):"},{"line_number":153,"context_line":"            sub_port \u003d SubPort(self._context, trunk_id\u003dself.id, **port_dict)"},{"line_number":154,"context_line":"            sub_port.create()"},{"line_number":155,"context_line":"            return sub_port"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_b83124ef","line":152,"in_reply_to":"dab17558_912a5d7c","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":149,"context_line":"                                         `segmentation_type` is not supported"},{"line_number":150,"context_line":"        \"\"\""},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"        def _create_sub_port(port_dict):"},{"line_number":153,"context_line":"            sub_port \u003d SubPort(self._context, trunk_id\u003dself.id, **port_dict)"},{"line_number":154,"context_line":"            sub_port.create()"},{"line_number":155,"context_line":"            return sub_port"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_912a5d7c","line":152,"in_reply_to":"dab17558_9d29623a","updated":"2016-05-18 01:07:38.000000000","message":"Striking a good modularity balance is tricky. In this case this test doesn\u0027t seem it\u0027s adding any value as it\u0027s simply creating an object and returning it, even though it\u0027s used in a comprehension. That said this is mainly syntactic sugar. My suggestion would be to try nail down the interface (method signature) and its body only to assess whether it\u0027s worth breaking it down further.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":57,"context_line":"def create_trunk(context, port_id, sub_ports\u003dNone):"},{"line_number":58,"context_line":"    \"\"\"Create :class:`Trunk` object for a given `port_id`."},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    This is a helper method that verifies if the specified port can be"},{"line_number":61,"context_line":"    used as a trunk and then creates new :class:`Trunk` object."},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    The `tenant_id` field value is inherited from the :param:`context`."}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_eb669c4f","line":60,"updated":"2016-05-21 00:27:08.000000000","message":"This is effectively a factory method, and it could be a classmethod for the Trunk.\n\nI must admit the asymmetry that these method introduce bothers me a little. I wish we could make them class or object methods like the other in this module.","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":82,"context_line":""},{"line_number":83,"context_line":"    with db_api.autonested_transaction(context.session):"},{"line_number":84,"context_line":"        try:"},{"line_number":85,"context_line":"            trunk.create()"},{"line_number":86,"context_line":"        except base.NeutronDbObjectDuplicateEntry:"},{"line_number":87,"context_line":"            raise t_exc.InvalidPort(port_id\u003dport_id, trunk_id\u003d\u0027\u003cNEW\u003e\u0027)"},{"line_number":88,"context_line":"        except o_exc.DBReferenceError:"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_eb37bcfc","line":85,"updated":"2016-05-21 00:27:08.000000000","message":"what session is this using?","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        try:"},{"line_number":85,"context_line":"            trunk.create()"},{"line_number":86,"context_line":"        except base.NeutronDbObjectDuplicateEntry:"},{"line_number":87,"context_line":"            raise t_exc.InvalidPort(port_id\u003dport_id, trunk_id\u003d\u0027\u003cNEW\u003e\u0027)"},{"line_number":88,"context_line":"        except o_exc.DBReferenceError:"},{"line_number":89,"context_line":"            raise n_exc.PortNotFound(port_id\u003dport_id)"},{"line_number":90,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_2b70a4db","line":87,"updated":"2016-05-21 00:27:08.000000000","message":"The fact that you\u0027re passing a dummy value for the trunk_id _smells_. My suggestion would be to expose \u0027port_id\u0027 and \u0027reason\u0027 instead. In one case you\u0027d do:\n\nt_exc.InvalidPort(port_id\u003dport_id, \"port in used by another trunk\")\n\nin the other case you\u0027d do:\n\nt_exc.InvalidPort(port_id\u003dport_id, \"port is already used by the specified trunk\")","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":152,"context_line":"                seg_type \u003d sub_port_params[\u0027segmentation_type\u0027]"},{"line_number":153,"context_line":"                seg_id \u003d sub_port_params[\u0027segmentation_id\u0027]"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"                check_seg \u003d t_const.SEGMENTATION_ID_VALIDATORS.get(seg_type)"},{"line_number":156,"context_line":"                if not(check_seg and check_seg(seg_id)):"},{"line_number":157,"context_line":"                    raise t_exc.InvalidSegment(trunk_id\u003dself.id,"},{"line_number":158,"context_line":"                                               **sub_port_params)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_ae5dc216","line":155,"updated":"2016-05-21 00:27:08.000000000","message":"I was going over the API validation [1] and we need to make a decision on what to expect the required validation that the DB is supposed to do. I am having second thoughts that the segmentation type validation belongs to the DB. If so we could have done that with an enum field for the type column. Furthermore, type and id may even be omitted from the API and could fed by the suport\u0027s network via business logic, in which case the segmentation type is going to be valid. \n\nIn a nutshell: I think we should expect here that any segmentation type given is valid and thus offload the validation to the caller. It\u0027ll decouple things more nicely. Also, consider using full names for variables, so seg_type -\u003e segmentation_type and seg_id -\u003e segmentation_id\n\n[1] https://review.openstack.org/#/c/319386/","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"1ac35dd5e107ef9e0ae4baa2efa46ef43aebcfdc","unresolved":false,"context_lines":[{"line_number":152,"context_line":"                seg_type \u003d sub_port_params[\u0027segmentation_type\u0027]"},{"line_number":153,"context_line":"                seg_id \u003d sub_port_params[\u0027segmentation_id\u0027]"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"                check_seg \u003d t_const.SEGMENTATION_ID_VALIDATORS.get(seg_type)"},{"line_number":156,"context_line":"                if not(check_seg and check_seg(seg_id)):"},{"line_number":157,"context_line":"                    raise t_exc.InvalidSegment(trunk_id\u003dself.id,"},{"line_number":158,"context_line":"                                               **sub_port_params)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_e145db6e","line":155,"in_reply_to":"bab6814e_497ea56a","updated":"2016-05-23 18:05:08.000000000","message":"No, mine was an hypothetical statement.","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"097a25bbf0f6def395fc820e3176c2bc8c521aea","unresolved":false,"context_lines":[{"line_number":152,"context_line":"                seg_type \u003d sub_port_params[\u0027segmentation_type\u0027]"},{"line_number":153,"context_line":"                seg_id \u003d sub_port_params[\u0027segmentation_id\u0027]"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"                check_seg \u003d t_const.SEGMENTATION_ID_VALIDATORS.get(seg_type)"},{"line_number":156,"context_line":"                if not(check_seg and check_seg(seg_id)):"},{"line_number":157,"context_line":"                    raise t_exc.InvalidSegment(trunk_id\u003dself.id,"},{"line_number":158,"context_line":"                                               **sub_port_params)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_497ea56a","line":155,"in_reply_to":"bab6814e_ae5dc216","updated":"2016-05-23 12:45:02.000000000","message":"\u003e If so we could have done that with an enum field for the type column.\n\nDo you mean that we should use OVO EnumField? In that case we\u0027d have to bump OVO version each time we need to support another segmentation_type (OVO hash changes depending on the allowed enum values). And we need to validate segmentation_id depending on the segmentation_type value, so we\u0027ll need a custom validator for that anyway. And another \u0027minor inconvenience\u0027 is that the current tests do not support OVO EnumField type (but we could fix that ofc).","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":153,"context_line":"                seg_id \u003d sub_port_params[\u0027segmentation_id\u0027]"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"                check_seg \u003d t_const.SEGMENTATION_ID_VALIDATORS.get(seg_type)"},{"line_number":156,"context_line":"                if not(check_seg and check_seg(seg_id)):"},{"line_number":157,"context_line":"                    raise t_exc.InvalidSegment(trunk_id\u003dself.id,"},{"line_number":158,"context_line":"                                               **sub_port_params)"},{"line_number":159,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_0e84ce5b","line":156,"updated":"2016-05-21 00:27:08.000000000","message":"check_seg(seg_id): what is it supposed to mean?","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":155,"context_line":"                check_seg \u003d t_const.SEGMENTATION_ID_VALIDATORS.get(seg_type)"},{"line_number":156,"context_line":"                if not(check_seg and check_seg(seg_id)):"},{"line_number":157,"context_line":"                    raise t_exc.InvalidSegment(trunk_id\u003dself.id,"},{"line_number":158,"context_line":"                                               **sub_port_params)"},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"                if Trunk.get_objects(self._context, port_id\u003dport_id):"},{"line_number":161,"context_line":"                    raise t_exc.InvalidPort(trunk_id\u003dself.id, port_id\u003dport_id)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_6e959a85","line":158,"updated":"2016-05-21 00:27:08.000000000","message":"do not pack args like things, if we extend the model and forgot to tweak the exceptions things will explode unexpectedly.\n\nAlso, my suggestion is to use port_id and reason as parameters (not sure that trunk_id is useful as it would be known to the caller)","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":157,"context_line":"                    raise t_exc.InvalidSegment(trunk_id\u003dself.id,"},{"line_number":158,"context_line":"                                               **sub_port_params)"},{"line_number":159,"context_line":""},{"line_number":160,"context_line":"                if Trunk.get_objects(self._context, port_id\u003dport_id):"},{"line_number":161,"context_line":"                    raise t_exc.InvalidPort(trunk_id\u003dself.id, port_id\u003dport_id)"},{"line_number":162,"context_line":""},{"line_number":163,"context_line":"                sub_port \u003d SubPort(self._context, trunk_id\u003dself.id,"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_4e3ff686","line":160,"updated":"2016-05-21 00:27:08.000000000","message":"My suggestion would be to tweak InvalidPort\u0027s params and accept the port and a reason. In this case the reason for invalid port is because the subport is already parent in another trunk (e.g. nested is not allowed).","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                    sub_port.create()"},{"line_number":168,"context_line":"                except o_exc.DBReferenceError:"},{"line_number":169,"context_line":"                    raise n_exc.PortNotFound(port_id\u003dsub_port.port_id)"},{"line_number":170,"context_line":"                except base.NeutronDbObjectDuplicateEntry:"},{"line_number":171,"context_line":"                    if is_port_used_by_trunk(self._context, port_id):"},{"line_number":172,"context_line":"                        raise t_exc.InvalidPort(trunk_id\u003dself.id,"},{"line_number":173,"context_line":"                                                port_id\u003dport_id)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_ae5022d5","line":170,"updated":"2016-05-21 00:27:08.000000000","message":"is_port_used_by_trunk would do another two lookups, is there a chance we can inspect the integrity violation error? In which case we can easily discern whether the error is due to duplicate subport or duplicate id.","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"06fcd1e9ad0a456cc150c831a5413a82eff9613f","unresolved":false,"context_lines":[{"line_number":172,"context_line":"                        raise t_exc.InvalidPort(trunk_id\u003dself.id,"},{"line_number":173,"context_line":"                                                port_id\u003dport_id)"},{"line_number":174,"context_line":"                    raise t_exc.InvalidSegment(trunk_id\u003dself.id,"},{"line_number":175,"context_line":"                                               **sub_port_params)"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"                new_sub_ports.append(sub_port)"},{"line_number":178,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"bab6814e_ee0b4ad4","line":175,"updated":"2016-05-21 00:27:08.000000000","message":"see line 156","commit_id":"dc12cf18b5492e8b0f794b1d75164e18531a8243"}],"neutron/services/trunk/const.py":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"3c27c3a5e1e7804dfc5cf4b46f62bad30b25d6d0","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2016 Mirantis, Inc."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_c9c96a5c","line":1,"updated":"2016-05-04 18:15:10.000000000","message":"I\u0027d call this constants, for readability but that\u0027s just me.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"2906c3e3781b13cee80f88a1c6af0c1d12812257","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# Copyright (c) 2016 Mirantis, Inc."},{"line_number":2,"context_line":"# All Rights Reserved."},{"line_number":3,"context_line":"#"},{"line_number":4,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_5195efe1","line":1,"in_reply_to":"1a122d0e_c9c96a5c","updated":"2016-05-05 18:57:12.000000000","message":"fixed","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"3c27c3a5e1e7804dfc5cf4b46f62bad30b25d6d0","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"SEGMENTATION_TYPE_VLAN \u003d \u0027vlan\u0027"},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"SEGMENTATION_TYPES \u003d ["},{"line_number":19,"context_line":"    SEGMENTATION_TYPE_VLAN"},{"line_number":20,"context_line":"]"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_29b8dec6","line":18,"updated":"2016-05-04 18:15:10.000000000","message":"consider using a tuple instead.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"2906c3e3781b13cee80f88a1c6af0c1d12812257","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"SEGMENTATION_TYPE_VLAN \u003d \u0027vlan\u0027"},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"SEGMENTATION_TYPES \u003d ["},{"line_number":19,"context_line":"    SEGMENTATION_TYPE_VLAN"},{"line_number":20,"context_line":"]"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_6d746bce","line":18,"in_reply_to":"1a122d0e_29b8dec6","updated":"2016-05-05 18:57:12.000000000","message":"fixed","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"}],"neutron/services/trunk/db.py":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"8d0a9aa0e4991da78d7a1bf1f1cf9b3dfb7dbb51","unresolved":false,"context_lines":[{"line_number":21,"context_line":"from neutron.services.trunk import models"},{"line_number":22,"context_line":""},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"def create_trunk(context, port_id, description\u003dNone):"},{"line_number":25,"context_line":"    \"\"\"Create a trunk (with description) given the parent port uuid.\"\"\""},{"line_number":26,"context_line":"    try:"},{"line_number":27,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_447d131b","side":"PARENT","line":24,"updated":"2016-04-27 13:09:49.000000000","message":"do you realize that we\u0027re stepping on each other toes? I appreciate the initiative you\u0027re taking, but your approach of unilaterally going on and redoing what was started with [1] is not gonna make us go any faster.\n\n[1] https://review.openstack.org/#/c/279251/","commit_id":"b047e3c28a0f3b714302dec17dcb0c138b758d90"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"c3e8853d2ed5a1140dfb36ba3f22578a9294bf57","unresolved":false,"context_lines":[{"line_number":21,"context_line":"from neutron.services.trunk import models"},{"line_number":22,"context_line":""},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"def create_trunk(context, port_id, description\u003dNone):"},{"line_number":25,"context_line":"    \"\"\"Create a trunk (with description) given the parent port uuid.\"\"\""},{"line_number":26,"context_line":"    try:"},{"line_number":27,"context_line":"        with context.session.begin(subtransactions\u003dTrue):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_1acff056","side":"PARENT","line":24,"in_reply_to":"1a122d0e_447d131b","updated":"2016-04-27 14:30:59.000000000","message":"I noticed that it was mentioned in [1] that we need to support versioned objects:\n\n\u003e We\u0027ll need to go to a versioned object approach before this finally lands however\n\nThere was another take on OVO support for VLAN-aware-VMs [2], but since I\u0027m also updating the model and was not sure if others would agree with that change, I started from scratch.\n\nSorry, I did not realise that this file was added by you recently. I thought it was part of Brent Eagles\u0027 patch. So I removed it as I thought it would be redundant to have both OVO and SA implementations at the same time.\n\n[1] https://review.openstack.org/#/c/281723/\n[2] https://review.openstack.org/#/c/296713","commit_id":"b047e3c28a0f3b714302dec17dcb0c138b758d90"}],"neutron/services/trunk/exceptions.py":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"3c27c3a5e1e7804dfc5cf4b46f62bad30b25d6d0","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"PortNotFound \u003d n_exc.PortNotFound"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class SubPortInUse(n_exc.PortInUse):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_0977e202","line":22,"updated":"2016-05-04 18:15:10.000000000","message":"I\u0027d avoid this alias.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"2906c3e3781b13cee80f88a1c6af0c1d12812257","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"PortNotFound \u003d n_exc.PortNotFound"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class SubPortInUse(n_exc.PortInUse):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_f1c19b4f","line":22,"in_reply_to":"1a122d0e_0977e202","updated":"2016-05-05 18:57:12.000000000","message":"fixed","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"3c27c3a5e1e7804dfc5cf4b46f62bad30b25d6d0","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class SubPortInUse(n_exc.PortInUse):"},{"line_number":26,"context_line":"    message \u003d _(\"Port %(port_id)s is in use by another sub-port.\")"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"class SubPortNotFound(n_exc.NotFound):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_49ed5a8d","line":26,"updated":"2016-05-04 18:15:10.000000000","message":"I\u0027d say: Port %(port_id)s is in use by another trunk.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"945e83355f7687f4d5999c9b3a41cf76d72f39a8","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class SubPortInUse(n_exc.PortInUse):"},{"line_number":26,"context_line":"    message \u003d _(\"Port %(port_id)s is in use by another sub-port.\")"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"class SubPortNotFound(n_exc.NotFound):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_d242aaaa","line":26,"in_reply_to":"1a122d0e_49ed5a8d","updated":"2016-05-04 21:28:10.000000000","message":"@Armando, I\u0027m not sure I follow. That is the message for TrunkPortInUse. This is the exception for when a subport already references a particular port, i.e. adding the same port twice in a \"trunk-subport-add\" command.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"20c028975232e743cf24db7b20a7d3f805d745fe","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class SubPortInUse(n_exc.PortInUse):"},{"line_number":26,"context_line":"    message \u003d _(\"Port %(port_id)s is in use by another sub-port.\")"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"class SubPortNotFound(n_exc.NotFound):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_f5a047f5","line":26,"in_reply_to":"1a122d0e_d242aaaa","updated":"2016-05-05 16:36:15.000000000","message":"right, I think we have two slightly different exceptions:\n\na) the port used to create a subport is already assigned in the same trunk\nb) the port used to create a subport is already assigned to another trunk\n\nI guess this message is fine, but perhaps passing the trunk id can help us distinguish the two.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"2906c3e3781b13cee80f88a1c6af0c1d12812257","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"class SubPortInUse(n_exc.PortInUse):"},{"line_number":26,"context_line":"    message \u003d _(\"Port %(port_id)s is in use by another sub-port.\")"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"class SubPortNotFound(n_exc.NotFound):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_2d5b9387","line":26,"in_reply_to":"1a122d0e_f5a047f5","updated":"2016-05-05 18:57:12.000000000","message":"@Armando, good point. fixed.","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"3c27c3a5e1e7804dfc5cf4b46f62bad30b25d6d0","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"class TrunkNotFound(n_exc.NotFound):"},{"line_number":34,"context_line":"    message \u003d _(\"Trunk %(trunk_id)s can not be found.\")"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"class TrunkPortNotFound(TrunkNotFound):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_c9532a6b","line":34,"updated":"2016-05-04 18:15:10.000000000","message":"cannot","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"2906c3e3781b13cee80f88a1c6af0c1d12812257","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"class TrunkNotFound(n_exc.NotFound):"},{"line_number":34,"context_line":"    message \u003d _(\"Trunk %(trunk_id)s can not be found.\")"},{"line_number":35,"context_line":""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"class TrunkPortNotFound(TrunkNotFound):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_917e6719","line":34,"in_reply_to":"1a122d0e_c9532a6b","updated":"2016-05-05 18:57:12.000000000","message":"fixed","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"3c27c3a5e1e7804dfc5cf4b46f62bad30b25d6d0","unresolved":false,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"class TrunkPortNotFound(TrunkNotFound):"},{"line_number":38,"context_line":"    message \u003d _(\"Trunk for port %(port_id)s can not be found.\")"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"class TrunkPortInUse(n_exc.PortInUse):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_89d012cb","line":38,"updated":"2016-05-04 18:15:10.000000000","message":"cannot","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"2906c3e3781b13cee80f88a1c6af0c1d12812257","unresolved":false,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"class TrunkPortNotFound(TrunkNotFound):"},{"line_number":38,"context_line":"    message \u003d _(\"Trunk for port %(port_id)s can not be found.\")"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"class TrunkPortInUse(n_exc.PortInUse):"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_d17c5f22","line":38,"in_reply_to":"1a122d0e_89d012cb","updated":"2016-05-05 18:57:12.000000000","message":"fixed","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":14,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from neutron_lib import exceptions as n_exc"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_314ce76f","line":17,"updated":"2016-05-13 22:16:13.000000000","message":"I\u0027d revise these exceptions based on the comments I provided in object/trunk.py\n\nI think we can simply have a\n\nInvalidPortForTrunk with uuid and reason","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":14,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from neutron_lib import exceptions as n_exc"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_d17ce572","line":17,"in_reply_to":"dab17558_1ded72ef","updated":"2016-05-18 01:07:38.000000000","message":"A test is a test. You need to test a code path and it doesn\u0027t matter if you have 1 or 100 exception classes. 1 is definitely easier to reason about remember.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":14,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from neutron_lib import exceptions as n_exc"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_1ded72ef","line":17,"in_reply_to":"dab17558_314ce76f","updated":"2016-05-14 01:28:21.000000000","message":"I actually like the idea of having separate exception for each specific case as it allows tests to be explicit (as long as we can have limited number of base exception classes to be used by callers, like {Trunk,Sub}PortInUse).\n\nSo I\u0027d like to keep them as-is, but use e.g. InvalidPortForTrunk as a base class and will update docstrings accordingly.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"85ce65ca03dac5c2c359ca0ee466a2eed9cad849","unresolved":false,"context_lines":[{"line_number":14,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from neutron_lib import exceptions as n_exc"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_34698abf","line":17,"in_reply_to":"dab17558_d17ce572","updated":"2016-06-06 21:48:46.000000000","message":"I agree that you need to test a code path regardless of what exception classes exist.  But, I\u0027m not sure I get the motivation to collapse them all in to one.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"class InvalidSegmentationType(n_exc.BadRequest):"},{"line_number":23,"context_line":"    message \u003d _(\"Trunk segmentation type %(segmentation_type)s is not \""},{"line_number":24,"context_line":"                \"supported.\")"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_5177eb9e","line":22,"updated":"2016-05-13 22:16:13.000000000","message":"We could perhaps collapse this in InvalidSegmentation and we can provide a reason on whether it\u0027s the type or the id that\u0027s invalid or in use.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"85ce65ca03dac5c2c359ca0ee466a2eed9cad849","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"class InvalidSegmentationType(n_exc.BadRequest):"},{"line_number":23,"context_line":"    message \u003d _(\"Trunk segmentation type %(segmentation_type)s is not \""},{"line_number":24,"context_line":"                \"supported.\")"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_92033142","line":22,"in_reply_to":"bab6814e_9896e092","updated":"2016-06-06 21:48:46.000000000","message":"I\u0027m not sure I agree with conflating these two.  But, I can\u0027t find InvalidSegmentation so I can\u0027t comment.  But, in general, if we have to use different reasons to distinguish between such different exceptional cases then I\u0027d personally rather have two exception classes.  What is the drawback?","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from neutron._i18n import _"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"class InvalidSegmentationType(n_exc.BadRequest):"},{"line_number":23,"context_line":"    message \u003d _(\"Trunk segmentation type %(segmentation_type)s is not \""},{"line_number":24,"context_line":"                \"supported.\")"},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_9896e092","line":22,"in_reply_to":"dab17558_5177eb9e","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    message \u003d _(\"Port %(port_id)s does not belong to trunk %(trunk_id)s.\")"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"class TrunkNotFound(n_exc.NotFound):"},{"line_number":36,"context_line":"    message \u003d _(\"Trunk %(trunk_id)s cannot be found.\")"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_a4dd3344","line":35,"updated":"2016-05-13 22:16:13.000000000","message":"As far as I can tell this was not used anywhere and having it as an intermediary of TrunkPortNotFound doesn\u0027t buy us anything","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"85ce65ca03dac5c2c359ca0ee466a2eed9cad849","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    message \u003d _(\"Port %(port_id)s does not belong to trunk %(trunk_id)s.\")"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"class TrunkNotFound(n_exc.NotFound):"},{"line_number":36,"context_line":"    message \u003d _(\"Trunk %(trunk_id)s cannot be found.\")"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_14360ee7","line":35,"in_reply_to":"bab6814e_58f3d80a","updated":"2016-06-06 21:48:46.000000000","message":"I agree with this.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    message \u003d _(\"Port %(port_id)s does not belong to trunk %(trunk_id)s.\")"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"class TrunkNotFound(n_exc.NotFound):"},{"line_number":36,"context_line":"    message \u003d _(\"Trunk %(trunk_id)s cannot be found.\")"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_58f3d80a","line":35,"in_reply_to":"dab17558_a4dd3344","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    message \u003d _(\"Trunk %(trunk_id)s cannot be found.\")"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"class TrunkPortNotFound(TrunkNotFound):"},{"line_number":40,"context_line":"    message \u003d _(\"Trunk for port %(port_id)s cannot be found.\")"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_f1c41f93","line":39,"updated":"2016-05-13 22:16:13.000000000","message":"not sure if we need two exceptions between TrunkNotFound and TrunkPortNotFound","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"85ce65ca03dac5c2c359ca0ee466a2eed9cad849","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    message \u003d _(\"Trunk %(trunk_id)s cannot be found.\")"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"class TrunkPortNotFound(TrunkNotFound):"},{"line_number":40,"context_line":"    message \u003d _(\"Trunk for port %(port_id)s cannot be found.\")"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_92cb3153","line":39,"in_reply_to":"bab6814e_d8e7c83e","updated":"2016-06-06 21:48:46.000000000","message":"I agree these could be collapsed since the former was never used anyway.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    message \u003d _(\"Trunk %(trunk_id)s cannot be found.\")"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"class TrunkPortNotFound(TrunkNotFound):"},{"line_number":40,"context_line":"    message \u003d _(\"Trunk for port %(port_id)s cannot be found.\")"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_d8e7c83e","line":39,"in_reply_to":"dab17558_f1c41f93","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"class TrunkSegmentInUse(n_exc.InUse):"},{"line_number":48,"context_line":"    message \u003d _(\"Segment type\u003d%(segmentation_type)s id\u003d%(segmentation_id)s \""},{"line_number":49,"context_line":"                \"of trunk %(trunk_id)s is in use by port %(port_id)s.\")"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_9112f307","line":49,"updated":"2016-05-13 22:16:13.000000000","message":"My suggestion would be to try and simplify the number of exceptions needed so far. The more exceptions there are the more test case we\u0027d have to write :)","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"85ce65ca03dac5c2c359ca0ee466a2eed9cad849","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"class TrunkSegmentInUse(n_exc.InUse):"},{"line_number":48,"context_line":"    message \u003d _(\"Segment type\u003d%(segmentation_type)s id\u003d%(segmentation_id)s \""},{"line_number":49,"context_line":"                \"of trunk %(trunk_id)s is in use by port %(port_id)s.\")"}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_b28f4dcb","line":49,"in_reply_to":"bab6814e_18d03053","updated":"2016-06-06 21:48:46.000000000","message":"I don\u0027t agree with \"the more exceptions there are the more test cases we\u0027d have to write\" as it is written.  If we use the number of exceptions classes as a general rule for how many tests we have to write then I think we\u0027ve done something wrong.\n\nLet me try to rephrase it to something I could believe:  \"the more exceptional cases there are the more test cases we\u0027d have to write.\"  The number of exception classes isn\u0027t the same as the number of exceptional code paths and simplifying the number of exception classes we have doesn\u0027t mean you can write fewer tests!\n\nI\u0027d say if we try to reuse the same exception class for more than one exceptional case and it ends up confusing the end user or developers, then we\u0027ve got a problem.  Since it takes very few lines of code to have a distinct exception class, I don\u0027t see a problem with making two different ones to help distinguish between different exceptional code paths.\n\nMy $0.02","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"class TrunkSegmentInUse(n_exc.InUse):"},{"line_number":48,"context_line":"    message \u003d _(\"Segment type\u003d%(segmentation_type)s id\u003d%(segmentation_id)s \""},{"line_number":49,"context_line":"                \"of trunk %(trunk_id)s is in use by port %(port_id)s.\")"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_fde7ce0f","line":49,"in_reply_to":"dab17558_9112f307","updated":"2016-05-14 01:28:21.000000000","message":"It would not affect the number of test cases as the cases are different :)","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4beb1b5c94e0a1a6cb39f1996928cdffbb08fdc2","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"class TrunkSegmentInUse(n_exc.InUse):"},{"line_number":48,"context_line":"    message \u003d _(\"Segment type\u003d%(segmentation_type)s id\u003d%(segmentation_id)s \""},{"line_number":49,"context_line":"                \"of trunk %(trunk_id)s is in use by port %(port_id)s.\")"}],"source_content_type":"text/x-python","patch_set":12,"id":"bab6814e_18d03053","line":49,"in_reply_to":"dab17558_d1ca259f","updated":"2016-05-18 12:45:46.000000000","message":"fixed","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"class TrunkSegmentInUse(n_exc.InUse):"},{"line_number":48,"context_line":"    message \u003d _(\"Segment type\u003d%(segmentation_type)s id\u003d%(segmentation_id)s \""},{"line_number":49,"context_line":"                \"of trunk %(trunk_id)s is in use by port %(port_id)s.\")"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_d1ca259f","line":49,"in_reply_to":"dab17558_fde7ce0f","updated":"2016-05-18 01:07:38.000000000","message":"Testing depends on the code paths yo\u0027d like to cover. If your aim is to have an exception for each single code paths we\u0027ll go insane","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"}],"neutron/services/trunk/models.py":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"8d0a9aa0e4991da78d7a1bf1f1cf9b3dfb7dbb51","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                         sa.ForeignKey(\u0027trunks.id\u0027,"},{"line_number":51,"context_line":"                                       ondelete\u003d\u0027CASCADE\u0027),"},{"line_number":52,"context_line":"                         nullable\u003dFalse,"},{"line_number":53,"context_line":"                         primary_key\u003dTrue)"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    segmentation_type \u003d sa.Column(sa.String(32), nullable\u003dFalse)"},{"line_number":56,"context_line":"    segmentation_id \u003d sa.Column(sa.Integer, nullable\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_a4cce735","side":"PARENT","line":53,"updated":"2016-04-27 13:09:49.000000000","message":"why are you dropping trunk_id from the primary key? Looks like this is not in the schema migration.","commit_id":"b047e3c28a0f3b714302dec17dcb0c138b758d90"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4afa49f6c3a163f4bfa26ecd31f8b0a8f330706b","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                         sa.ForeignKey(\u0027trunks.id\u0027,"},{"line_number":51,"context_line":"                                       ondelete\u003d\u0027CASCADE\u0027),"},{"line_number":52,"context_line":"                         nullable\u003dFalse,"},{"line_number":53,"context_line":"                         primary_key\u003dTrue)"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    segmentation_type \u003d sa.Column(sa.String(32), nullable\u003dFalse)"},{"line_number":56,"context_line":"    segmentation_id \u003d sa.Column(sa.Integer, nullable\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_ee434740","side":"PARENT","line":53,"in_reply_to":"1a122d0e_220efd8b","updated":"2016-04-28 04:30:38.000000000","message":"\u003e If you want to take over the DB work, and can devote time and energy to it\n\nDon\u0027t get me wrong, I did not intend to take over anything :) The primary purpose of this submission was to get in touch with others to discuss the changes in DB model.\n\nRight now I have some rather coarse/partial implementation for the most parts:\n\n- neutron client from [1]\n\n- server API from [2]\n\n- ovo\n\n- service plugin (i.e. server API impl)\n\n- resource-based rpc (similar to QoS)\n\n- agent extension (similar to QoS)\n\n- drivers for agent extension (plug/unplug; only vlan@tap for ovs-native/noop-firewall right now, as the discussion is not finished yet)\n\nSo I was intending to get it into \u0027mostly-complete\u0027 state and submit as WIPs and since OVO is used by all the other parts, I wanted to get feedback on it ASAP.\n\n[1] https://review.openstack.org/#/c/283407\n[2] https://review.openstack.org/#/c/281723","commit_id":"b047e3c28a0f3b714302dec17dcb0c138b758d90"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"35c47ab6f0c8956a9113c04dbd86ab3ce3a09e65","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                         sa.ForeignKey(\u0027trunks.id\u0027,"},{"line_number":51,"context_line":"                                       ondelete\u003d\u0027CASCADE\u0027),"},{"line_number":52,"context_line":"                         nullable\u003dFalse,"},{"line_number":53,"context_line":"                         primary_key\u003dTrue)"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    segmentation_type \u003d sa.Column(sa.String(32), nullable\u003dFalse)"},{"line_number":56,"context_line":"    segmentation_id \u003d sa.Column(sa.Integer, nullable\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_220efd8b","side":"PARENT","line":53,"in_reply_to":"1a122d0e_9fe2d2d3","updated":"2016-04-27 19:51:49.000000000","message":"Don\u0027t, because I don\u0027t think these DB migrations are needed :)\n\nI am suggesting to revert the model changes and work within the boundaries that we set. If you want to take over the DB work, and can devote time and energy to it I am happy to support your effort with review attention on my part.","commit_id":"b047e3c28a0f3b714302dec17dcb0c138b758d90"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"c3e8853d2ed5a1140dfb36ba3f22578a9294bf57","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                         sa.ForeignKey(\u0027trunks.id\u0027,"},{"line_number":51,"context_line":"                                       ondelete\u003d\u0027CASCADE\u0027),"},{"line_number":52,"context_line":"                         nullable\u003dFalse,"},{"line_number":53,"context_line":"                         primary_key\u003dTrue)"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    segmentation_type \u003d sa.Column(sa.String(32), nullable\u003dFalse)"},{"line_number":56,"context_line":"    segmentation_id \u003d sa.Column(sa.Integer, nullable\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a122d0e_9fe2d2d3","side":"PARENT","line":53,"in_reply_to":"1a122d0e_a4cce735","updated":"2016-04-27 14:30:59.000000000","message":"Thanks. Will update migration scripts in the next PS. Dropped it from PK because as far as I understand we are not supposed to have the same port bound to different trunks.","commit_id":"b047e3c28a0f3b714302dec17dcb0c138b758d90"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"f9c03f00c5b5dac72fa8cf03abdb7c06c740d5eb","unresolved":false,"context_lines":[{"line_number":41,"context_line":"                        nullable\u003dFalse,"},{"line_number":42,"context_line":"                        unique\u003dTrue,"},{"line_number":43,"context_line":"                        primary_key\u003dTrue)"},{"line_number":44,"context_line":"    port \u003d sa.orm.relationship("},{"line_number":45,"context_line":"        models_v2.Port,"},{"line_number":46,"context_line":"        backref\u003dsa.orm.backref(\u0027sub_port\u0027, lazy\u003d\u0027joined\u0027, uselist\u003dFalse,"},{"line_number":47,"context_line":"                               cascade\u003d\u0027delete\u0027))"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    trunk_id \u003d sa.Column(sa.String(36),"},{"line_number":50,"context_line":"                         sa.ForeignKey(\u0027trunks.id\u0027,"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_08d5d7e6","side":"PARENT","line":47,"range":{"start_line":44,"start_character":4,"end_line":47,"end_character":49},"updated":"2016-05-11 07:18:42.000000000","message":"Maybe I\u0027n not into subject, but why are you deleting the backref? It would delete the sub port when deleting the trunk port in DBMS, and instead you are doing it in OVO implementation... just wondering...","commit_id":"60db12f9d2f8a44d411e2ee8742c33be08078864"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"07e7d2fc9f2147b14857c65353f4922b113deff3","unresolved":false,"context_lines":[{"line_number":41,"context_line":"                        nullable\u003dFalse,"},{"line_number":42,"context_line":"                        unique\u003dTrue,"},{"line_number":43,"context_line":"                        primary_key\u003dTrue)"},{"line_number":44,"context_line":"    port \u003d sa.orm.relationship("},{"line_number":45,"context_line":"        models_v2.Port,"},{"line_number":46,"context_line":"        backref\u003dsa.orm.backref(\u0027sub_port\u0027, lazy\u003d\u0027joined\u0027, uselist\u003dFalse,"},{"line_number":47,"context_line":"                               cascade\u003d\u0027delete\u0027))"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    trunk_id \u003d sa.Column(sa.String(36),"},{"line_number":50,"context_line":"                         sa.ForeignKey(\u0027trunks.id\u0027,"}],"source_content_type":"text/x-python","patch_set":9,"id":"dab17558_d3b38408","side":"PARENT","line":47,"range":{"start_line":44,"start_character":4,"end_line":47,"end_character":49},"in_reply_to":"dab17558_08d5d7e6","updated":"2016-05-11 08:53:48.000000000","message":"Exactly for the reason that it has to be forbidden. Please see NOTE by @Armando after SubPort.","commit_id":"60db12f9d2f8a44d411e2ee8742c33be08078864"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":75,"context_line":"# A port cannot be a subport and a trunk port at the same time (nested)."},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"class Constraints(object):"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"    @classmethod"},{"line_number":81,"context_line":"    def _get_events(cls):"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_31fd073b","line":78,"updated":"2016-05-13 22:16:13.000000000","message":"consolidating constraints like you did is a good idea, though I have some reservations on the use of sqlalchemy events as outlined below. Let\u0027s see what other think, I would have stayed on the course you chose in PS7 but modelled constraints in a way that were easier to develop and maintain","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":75,"context_line":"# A port cannot be a subport and a trunk port at the same time (nested)."},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"class Constraints(object):"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"    @classmethod"},{"line_number":81,"context_line":"    def _get_events(cls):"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_c2a5c704","line":78,"in_reply_to":"dab17558_31fd073b","updated":"2016-05-14 01:28:21.000000000","message":"Well, I guess I misunderstood what you meant by \u0027hooks\u0027 then. But I noticed that we are using SA event hooks in some places and thought that\u0027s what you mean.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":75,"context_line":"# A port cannot be a subport and a trunk port at the same time (nested)."},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"class Constraints(object):"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"    @classmethod"},{"line_number":81,"context_line":"    def _get_events(cls):"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_d141850b","line":78,"in_reply_to":"dab17558_c2a5c704","updated":"2016-05-18 01:07:38.000000000","message":"SA events are used in a handful strategic places, but never for validation.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":78,"context_line":"class Constraints(object):"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"    @classmethod"},{"line_number":81,"context_line":"    def _get_events(cls):"},{"line_number":82,"context_line":"        return ("},{"line_number":83,"context_line":"            (SubPort, \u0027before_insert\u0027, cls.validate_sub_port),"},{"line_number":84,"context_line":"            (Trunk, \u0027before_insert\u0027, cls.validate_trunk),"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_6426abbb","line":81,"updated":"2016-05-13 22:16:13.000000000","message":"these hooks can easily go back into the trunk.py module and we can avoid relying on events, but these are good","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":78,"context_line":"class Constraints(object):"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"    @classmethod"},{"line_number":81,"context_line":"    def _get_events(cls):"},{"line_number":82,"context_line":"        return ("},{"line_number":83,"context_line":"            (SubPort, \u0027before_insert\u0027, cls.validate_sub_port),"},{"line_number":84,"context_line":"            (Trunk, \u0027before_insert\u0027, cls.validate_trunk),"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_6285fb57","line":81,"in_reply_to":"dab17558_6426abbb","updated":"2016-05-14 01:28:21.000000000","message":"They made sense here as SA-based checks as they are not really related to OVO but the model itself. But if the SA events are not to be used, then of course they belong to OVO.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":82,"context_line":"        return ("},{"line_number":83,"context_line":"            (SubPort, \u0027before_insert\u0027, cls.validate_sub_port),"},{"line_number":84,"context_line":"            (Trunk, \u0027before_insert\u0027, cls.validate_trunk),"},{"line_number":85,"context_line":"            (models_v2.Port, \u0027before_delete\u0027, cls.validate_port),"},{"line_number":86,"context_line":"        )"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_11ee03b4","line":85,"updated":"2016-05-13 22:16:13.000000000","message":"Doing validation on port delete is problematic as you\u0027re injecting code into the code path for port_delete and raising an error like TrunkInUse may as well lead to an unexpected error situation. We typically prevent certain operations by means of registry callbacks.\n\nIn that case, I\u0027d avoid the combination of sqlalchemy events and registry events\n\nHaving said that, we said we\u0027d like to forbid direct deletion of ports used in a trunk, and we can do by adding a hook in the port delete operations so that we have better control of the return error. Alternatively we can relax this constraint and ensure that the direct deletion of a trunk is handled correctly, though this can be done at a later date.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":82,"context_line":"        return ("},{"line_number":83,"context_line":"            (SubPort, \u0027before_insert\u0027, cls.validate_sub_port),"},{"line_number":84,"context_line":"            (Trunk, \u0027before_insert\u0027, cls.validate_trunk),"},{"line_number":85,"context_line":"            (models_v2.Port, \u0027before_delete\u0027, cls.validate_port),"},{"line_number":86,"context_line":"        )"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_e2e28b00","line":85,"in_reply_to":"dab17558_11ee03b4","updated":"2016-05-14 01:28:21.000000000","message":"same as L78.\n\nNow, to make sure there\u0027s no misunderstanding this time, should I use \u0027neutron.callbacks.resources\u0027 to handle port.delete (unfortunately I was not aware we got local callbacks in addition to rpc ones)? And I think those port-related constraints actually belong to the service plugin, but not the OVO itself. Are you ok with that?","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":100,"context_line":"                                \"not found\"), target\u003devent[0], event\u003devent[1])"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @staticmethod"},{"line_number":103,"context_line":"    def validate_sub_port(mapper, connection, sub_port):"},{"line_number":104,"context_line":"        seg_type \u003d sub_port.segmentation_type"},{"line_number":105,"context_line":"        seg_id \u003d sub_port.segmentation_id"},{"line_number":106,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_91b9537c","line":103,"updated":"2016-05-13 22:16:13.000000000","message":"I would have","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":100,"context_line":"                                \"not found\"), target\u003devent[0], event\u003devent[1])"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @staticmethod"},{"line_number":103,"context_line":"    def validate_sub_port(mapper, connection, sub_port):"},{"line_number":104,"context_line":"        seg_type \u003d sub_port.segmentation_type"},{"line_number":105,"context_line":"        seg_id \u003d sub_port.segmentation_id"},{"line_number":106,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_314d713e","line":103,"in_reply_to":"dab17558_02deaf6c","updated":"2016-05-18 01:07:38.000000000","message":"um I must have lost the train of thought :)","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":100,"context_line":"                                \"not found\"), target\u003devent[0], event\u003devent[1])"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"    @staticmethod"},{"line_number":103,"context_line":"    def validate_sub_port(mapper, connection, sub_port):"},{"line_number":104,"context_line":"        seg_type \u003d sub_port.segmentation_type"},{"line_number":105,"context_line":"        seg_id \u003d sub_port.segmentation_id"},{"line_number":106,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_02deaf6c","line":103,"in_reply_to":"dab17558_91b9537c","updated":"2016-05-14 01:28:21.000000000","message":"what?","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":113,"context_line":"        t_query \u003d sa.select([Trunk.id]).where(t_filter)"},{"line_number":114,"context_line":"        trunk_id \u003d connection.scalar(t_query)"},{"line_number":115,"context_line":"        if trunk_id is not None:"},{"line_number":116,"context_line":"            raise t_exc.TrunkPortInUse(trunk_id\u003dtrunk_id,"},{"line_number":117,"context_line":"                                       port_id\u003dsub_port.port_id)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        # check for duplicate port_id"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_c435d7db","line":116,"updated":"2016-05-13 22:16:13.000000000","message":"As suggested, I\u0027d consolidate the error condition and raise an InvalidPort with a reason, because there might be other reasons why the port is unable to be used.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":113,"context_line":"        t_query \u003d sa.select([Trunk.id]).where(t_filter)"},{"line_number":114,"context_line":"        trunk_id \u003d connection.scalar(t_query)"},{"line_number":115,"context_line":"        if trunk_id is not None:"},{"line_number":116,"context_line":"            raise t_exc.TrunkPortInUse(trunk_id\u003dtrunk_id,"},{"line_number":117,"context_line":"                                       port_id\u003dsub_port.port_id)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        # check for duplicate port_id"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_f10c89ed","line":116,"in_reply_to":"dab17558_674a2d85","updated":"2016-05-18 01:07:38.000000000","message":"fine, though agreeing to disagree it\u0027s not gonna help us getting to the end any faster, especially if I genuinely think I am in the right ;)","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":113,"context_line":"        t_query \u003d sa.select([Trunk.id]).where(t_filter)"},{"line_number":114,"context_line":"        trunk_id \u003d connection.scalar(t_query)"},{"line_number":115,"context_line":"        if trunk_id is not None:"},{"line_number":116,"context_line":"            raise t_exc.TrunkPortInUse(trunk_id\u003dtrunk_id,"},{"line_number":117,"context_line":"                                       port_id\u003dsub_port.port_id)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        # check for duplicate port_id"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_674a2d85","line":116,"in_reply_to":"dab17558_c435d7db","updated":"2016-05-14 01:28:21.000000000","message":"They are already subclasses of PortInUse but provide more refined details on what\u0027s going on. And I believe PortInUse fits as a base exception here, as the port is indeed in use :)","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                                          segmentation_type\u003dseg_type,"},{"line_number":137,"context_line":"                                          segmentation_id\u003dseg_id)"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"        # check if port exists"},{"line_number":140,"context_line":"        t_filter \u003d models_v2.Port.id \u003d\u003d sub_port.port_id"},{"line_number":141,"context_line":"        t_query \u003d sa.select([models_v2.Port.id]).where(t_filter)"},{"line_number":142,"context_line":"        port_id \u003d connection.scalar(t_query)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_51bfeb5f","line":139,"updated":"2016-05-13 22:16:13.000000000","message":"if this doesn\u0027t exist we\u0027d get a foreign key constraint violation so this seems superflous","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                                          segmentation_type\u003dseg_type,"},{"line_number":137,"context_line":"                                          segmentation_id\u003dseg_id)"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"        # check if port exists"},{"line_number":140,"context_line":"        t_filter \u003d models_v2.Port.id \u003d\u003d sub_port.port_id"},{"line_number":141,"context_line":"        t_query \u003d sa.select([models_v2.Port.id]).where(t_filter)"},{"line_number":142,"context_line":"        port_id \u003d connection.scalar(t_query)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_c735d9db","line":139,"in_reply_to":"dab17558_51bfeb5f","updated":"2016-05-14 01:28:21.000000000","message":"This check and the 2 before it are indeed covered by DB constraints, but I\u0027ve added them here so the trunk\u0027s exceptions are raised instead of DB ones for consistency reasons. While they do add a slight DB overhead, they are easier to maintain/understand than what I did with exception remapping in PS7.\n\nBut if you\u0027d say that we can leave the DB exceptions as-is, I could as well remove these and some other special exceptions.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                                          segmentation_type\u003dseg_type,"},{"line_number":137,"context_line":"                                          segmentation_id\u003dseg_id)"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"        # check if port exists"},{"line_number":140,"context_line":"        t_filter \u003d models_v2.Port.id \u003d\u003d sub_port.port_id"},{"line_number":141,"context_line":"        t_query \u003d sa.select([models_v2.Port.id]).where(t_filter)"},{"line_number":142,"context_line":"        port_id \u003d connection.scalar(t_query)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_b1896128","line":139,"in_reply_to":"dab17558_c735d9db","updated":"2016-05-18 01:07:38.000000000","message":"It seems a waste for no good reason if you ask me. Why would you want to maintain more code than it\u0027s strictly necessary?","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":160,"context_line":"            raise t_exc.TrunkPortInUse(trunk_id\u003dtrunk_id,"},{"line_number":161,"context_line":"                                       port_id\u003dtrunk.port_id)"},{"line_number":162,"context_line":""},{"line_number":163,"context_line":"        # check if port exists"},{"line_number":164,"context_line":"        t_filter \u003d models_v2.Port.id \u003d\u003d trunk.port_id"},{"line_number":165,"context_line":"        t_query \u003d sa.select([models_v2.Port.id]).where(t_filter)"},{"line_number":166,"context_line":"        port_id \u003d connection.scalar(t_query)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_847f4f65","line":163,"range":{"start_line":163,"start_character":14,"end_line":163,"end_character":15},"updated":"2016-05-13 22:16:13.000000000","message":"see line 139","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":160,"context_line":"            raise t_exc.TrunkPortInUse(trunk_id\u003dtrunk_id,"},{"line_number":161,"context_line":"                                       port_id\u003dtrunk.port_id)"},{"line_number":162,"context_line":""},{"line_number":163,"context_line":"        # check if port exists"},{"line_number":164,"context_line":"        t_filter \u003d models_v2.Port.id \u003d\u003d trunk.port_id"},{"line_number":165,"context_line":"        t_query \u003d sa.select([models_v2.Port.id]).where(t_filter)"},{"line_number":166,"context_line":"        port_id \u003d connection.scalar(t_query)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_c71b191b","line":163,"range":{"start_line":163,"start_character":14,"end_line":163,"end_character":15},"in_reply_to":"dab17558_847f4f65","updated":"2016-05-14 01:28:21.000000000","message":"ditto","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        t_query \u003d sa.select([SubPort.trunk_id]).where(t_filter)"},{"line_number":182,"context_line":"        trunk_id \u003d connection.scalar(t_query)"},{"line_number":183,"context_line":"        if trunk_id is not None:"},{"line_number":184,"context_line":"            raise t_exc.SubPortInUse(trunk_id\u003dtrunk_id, port_id\u003dport.id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_f1db7feb","line":184,"updated":"2016-05-13 22:16:13.000000000","message":"both conditions should be seen as a port in use by trunk. I don\u0027t see why we\u0027d make a difference from a caller point of view","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"294f471a035c080c607935698d875864523719fe","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        t_query \u003d sa.select([SubPort.trunk_id]).where(t_filter)"},{"line_number":182,"context_line":"        trunk_id \u003d connection.scalar(t_query)"},{"line_number":183,"context_line":"        if trunk_id is not None:"},{"line_number":184,"context_line":"            raise t_exc.SubPortInUse(trunk_id\u003dtrunk_id, port_id\u003dport.id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_b1de810e","line":184,"in_reply_to":"dab17558_472f293e","updated":"2016-05-18 01:07:38.000000000","message":"That\u0027s the second time you make this point, but I am not sure what you mean by \u0027explicit\u0027. You can test the code paths to ensure a good level of coverage; to this aim, it\u0027s not necessary to have each path be identified by a unique exception. I appreciate that exception subclassing helps narrow down error conditions but when it makes sense because error handling may differ.\n\nAgain, I am making the argument that a caller that receives the InUse exception has no reason of treating the two error conditions differently. The port is already employed in a trunk, and there\u0027s no recovery action (e.g. freeing up the resource to attempt a retry), so the caller is only left to fail.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"e023538a43ee069778ae062ec3d1029329510b3e","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        t_query \u003d sa.select([SubPort.trunk_id]).where(t_filter)"},{"line_number":182,"context_line":"        trunk_id \u003d connection.scalar(t_query)"},{"line_number":183,"context_line":"        if trunk_id is not None:"},{"line_number":184,"context_line":"            raise t_exc.SubPortInUse(trunk_id\u003dtrunk_id, port_id\u003dport.id)"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_472f293e","line":184,"in_reply_to":"dab17558_f1db7feb","updated":"2016-05-14 01:28:21.000000000","message":"Both SubPortInUse and TrunkPortInUse are subclasses of of PortInUse, so if the caller can just check for PortInUse, but would still have 2 different messages and that allows for a more explicit testing.","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"}],"neutron/services/trunk/objects.py":[{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"92ff38bee18a08e25ad46e9067e29656a3fb7e66","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"1a122d0e_b6908bee","updated":"2016-04-27 23:00:29.000000000","message":"From http://docs.openstack.org/developer/oslo.versionedobjects/usage.html#create-objects-subdirectory-and-a-base-py-inside-it I think versioned objects belong in the neutron/objects/ directory, where the other versioned objects reside as well.","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"4afa49f6c3a163f4bfa26ecd31f8b0a8f330706b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"1a122d0e_ce65abe1","in_reply_to":"1a122d0e_b6908bee","updated":"2016-04-28 04:30:38.000000000","message":"Thanks for the link. Already fixed, waiting for the next PS (+unit test fixes for test_objects). The intention was to have everything in one place, as it\u0027s a nightmare jumping through the whole project tree for the different parts of the same thing.","commit_id":"fca8a8d2dd1186871a57240aaa419dec1391349a"}],"neutron/tests/unit/objects/test_trunk.py":[{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"945e83355f7687f4d5999c9b3a41cf76d72f39a8","unresolved":false,"context_lines":[{"line_number":163,"context_line":""},{"line_number":164,"context_line":"    def test__delete_trunk(self):"},{"line_number":165,"context_line":"        trunk \u003d self._create_trunk()"},{"line_number":166,"context_line":"        trunk_id \u003d trunk.id"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"        sub_port_dicts \u003d [self._create_sub_port() for _ in range(3)]"},{"line_number":169,"context_line":"        trunk.attach_sub_ports(sub_port_dicts)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_f7e1989c","line":166,"range":{"start_line":166,"start_character":8,"end_line":166,"end_character":27},"updated":"2016-05-04 21:28:10.000000000","message":"pep8: \u0027trunk_id\u0027 is assigned to but never used","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"2906c3e3781b13cee80f88a1c6af0c1d12812257","unresolved":false,"context_lines":[{"line_number":163,"context_line":""},{"line_number":164,"context_line":"    def test__delete_trunk(self):"},{"line_number":165,"context_line":"        trunk \u003d self._create_trunk()"},{"line_number":166,"context_line":"        trunk_id \u003d trunk.id"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"        sub_port_dicts \u003d [self._create_sub_port() for _ in range(3)]"},{"line_number":169,"context_line":"        trunk.attach_sub_ports(sub_port_dicts)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_f18cfbd8","line":166,"range":{"start_line":166,"start_character":8,"end_line":166,"end_character":27},"in_reply_to":"1a122d0e_f7e1989c","updated":"2016-05-05 18:57:12.000000000","message":"fixed","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"945e83355f7687f4d5999c9b3a41cf76d72f39a8","unresolved":false,"context_lines":[{"line_number":264,"context_line":""},{"line_number":265,"context_line":"        trunk.attach_sub_ports(sub_port_dicts)"},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"        self.assertEqual(sorted(sub_port_dicts),"},{"line_number":268,"context_line":"                         sorted(self._sub_port_to_dict(p) for p in"},{"line_number":269,"context_line":"                                trunk.sub_ports))"},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    def test__detach_sub_port(self):"},{"line_number":272,"context_line":"        trunk \u003d self._create_trunk()"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_5784ccd1","line":269,"range":{"start_line":267,"start_character":24,"end_line":269,"end_character":49},"updated":"2016-05-04 21:28:10.000000000","message":"You might need to provide the comparator (cmp) and/or key parameters to the \"sorted\" function for it to work:\n\n \n    self.assertEqual(sorted(sub_port_dicts),\nTypeError: unorderable types: dict() \u003c dict()","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"2906c3e3781b13cee80f88a1c6af0c1d12812257","unresolved":false,"context_lines":[{"line_number":264,"context_line":""},{"line_number":265,"context_line":"        trunk.attach_sub_ports(sub_port_dicts)"},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"        self.assertEqual(sorted(sub_port_dicts),"},{"line_number":268,"context_line":"                         sorted(self._sub_port_to_dict(p) for p in"},{"line_number":269,"context_line":"                                trunk.sub_ports))"},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    def test__detach_sub_port(self):"},{"line_number":272,"context_line":"        trunk \u003d self._create_trunk()"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_31c153e5","line":269,"range":{"start_line":267,"start_character":24,"end_line":269,"end_character":49},"in_reply_to":"1a122d0e_5784ccd1","updated":"2016-05-05 18:57:12.000000000","message":"fixed","commit_id":"c387c9daa353564af6fa2f3c45a6e8608180eeca"},{"author":{"_account_id":21273,"name":"Ilya Chukhnakov","email":"Ilya.chukhnakov@gmail.com","username":"ichukhnakov"},"change_message_id":"9e6b3fc3648d88745eb5cbb609699f9e787d7525","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":"import oslo_versionedobjects.fields as ovo_fields"},{"line_number":18,"context_line":"from neutron_lib import exceptions as n_exc"},{"line_number":19,"context_line":"import random"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"from neutron import context"}],"source_content_type":"text/x-python","patch_set":4,"id":"1a122d0e_317e73e4","line":18,"range":{"start_line":18,"start_character":0,"end_line":18,"end_character":43},"updated":"2016-05-05 04:41:25.000000000","message":"will fix import order in the next PS","commit_id":"84408a3ebb8b3c3ce4e077481c801e4313489823"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":57,"context_line":"        self._id_seq \u003d 1"},{"line_number":58,"context_line":"        self._seg_seq \u003d 1"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def tearDown(self):"},{"line_number":61,"context_line":"        super(TrunkDbObjectTestCase, self).tearDown()"},{"line_number":62,"context_line":"        t_models.Constraints.unregister()"},{"line_number":63,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_04ba3f0a","line":60,"updated":"2016-05-13 22:16:13.000000000","message":"you should not use tearDown() but rather addCleanup(), having said that if you move away from sqlalchemy events you won\u0027t need to worry about this","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"b41b2c087dc508ff067f9cc22fc4cc4b64e09278","unresolved":false,"context_lines":[{"line_number":79,"context_line":"                                         \u0027device_owner\u0027: \u0027fake_owner\u0027})"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    @classmethod"},{"line_number":82,"context_line":"    def _sub_port_dict(cls, port_id, seg_type, seg_id):"},{"line_number":83,"context_line":"        return {\u0027port_id\u0027: port_id,"},{"line_number":84,"context_line":"                \u0027segmentation_type\u0027: seg_type,"},{"line_number":85,"context_line":"                \u0027segmentation_id\u0027: seg_id}"}],"source_content_type":"text/x-python","patch_set":12,"id":"dab17558_246dc398","line":82,"updated":"2016-05-13 22:16:13.000000000","message":"why do we need these if the trunk methods can handle objects?","commit_id":"10c008e8694d103fb341d5271db66bf084ba0115"},{"author":{"_account_id":15518,"name":"Artur Korzeniewski","email":"artur.korzen@wp.pl","username":"akorzeni"},"change_message_id":"1cc224d897724483db79e4e204a6405e288ad693","unresolved":false,"context_lines":[{"line_number":17,"context_line":"from neutron.tests.unit.objects import test_base"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"class SubPortObjectTestCase(test_base.BaseObjectIfaceTestCase):"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"    _test_class \u003d t_obj.SubPort"},{"line_number":23,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"bab6814e_1fd2da34","line":20,"range":{"start_line":20,"start_character":6,"end_line":20,"end_character":27},"updated":"2016-05-24 09:12:06.000000000","message":"No DbObjectTestCase for SubPort?","commit_id":"2ec87830574b7ee47f057662a75c2704219130bb"}]}