)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"cd4fd6377b4751480ae2033e922fcceea3f88179","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3b052f30_086b3fbc","updated":"2021-10-20 12:43:43.000000000","message":"Pulled all the bgpaas code into my devstack and with that I\u0027m getting to actually review these patches. Sorry for being slow.\n\nI believe we need a short and clear explanation (printed by --help) on the difference between the following two (when to use one and when to use the other):\n\nopenstack bgp speaker add peer\nopenstack bgp speaker peer association create\n\nWithout that our users will be confused.","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"77d9acc23113c7fce8bb2b2d923a47ae61065109","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"753079a8_7999501b","in_reply_to":"3b052f30_086b3fbc","updated":"2021-10-26 12:16:30.000000000","message":"I have added more explanation in the help section of \u0027peer association create\u0027. Please let me know if it is fine","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"cf5793f1f9b9b2a79fc2da2380502b5d986bffea","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"922b1d58_652fbb76","updated":"2021-11-15 05:36:26.000000000","message":"recheck","commit_id":"4798f19826f24770b0d79e315457a5e8e46f80a5"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"72e9acb06f2393b2564a769995310eac244dc578","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"cf872cfc_a9e0256f","updated":"2021-11-03 04:28:10.000000000","message":"recheck","commit_id":"4798f19826f24770b0d79e315457a5e8e46f80a5"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"5d55660148fa89483174571dc916310994d7ed71","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"e777aa4d_09c7975e","updated":"2021-10-28 06:41:27.000000000","message":"recheck","commit_id":"4798f19826f24770b0d79e315457a5e8e46f80a5"}],"neutronclient/osc/v2/dynamic_routing/peer_association.py":[{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"cd4fd6377b4751480ae2033e922fcceea3f88179","unresolved":true,"context_lines":[{"line_number":39,"context_line":"    def get_parser(self, prog_name):"},{"line_number":40,"context_line":"        parser \u003d super(CreateSpeakerPeerAssociation, self)\\"},{"line_number":41,"context_line":"                .get_parser(prog_name)"},{"line_number":42,"context_line":"        parser.add_argument("},{"line_number":43,"context_line":"            \u0027name\u0027,"},{"line_number":44,"context_line":"            metavar\u003d\u0027\u003cname\u003e\u0027,"},{"line_number":45,"context_line":"            help\u003d_(\"Name of the BGP speaker peer association to create\"))"},{"line_number":46,"context_line":"        parser.add_argument("},{"line_number":47,"context_line":"            \u0027bgp_speaker\u0027,"},{"line_number":48,"context_line":"            metavar\u003d\u0027BGP_SPEAKER\u0027,"},{"line_number":49,"context_line":"            help\u003d_(\u0027ID or name of the BGP speaker.\u0027))"},{"line_number":50,"context_line":"        parser.add_argument("},{"line_number":51,"context_line":"            \u0027bgp_peer\u0027,"},{"line_number":52,"context_line":"            metavar\u003d\u0027BGP_PEER\u0027,"},{"line_number":53,"context_line":"            help\u003d_(\u0027ID or name of the peer to add.\u0027))"},{"line_number":54,"context_line":"        nc_osc_utils.add_project_owner_option_to_parser(parser)"},{"line_number":55,"context_line":"        return parser"},{"line_number":56,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"cf64cb2c_480a9919","line":53,"range":{"start_line":42,"start_character":0,"end_line":53,"end_character":53},"updated":"2021-10-20 12:43:43.000000000","message":"I see that in the spec we turned the association objects into first class objects, but only halfway. They have (globally unique) IDs (and not unique names), but still they cannot be operated on without a reference to the bgp speaker. Which was probably a mistake (and unfortunately the same mistake we made with qos policy rules before), but I don\u0027t suggest we change the spec now.\n\nHowever here I recommend that we stick to the same order as we have in the url and (mostly) in the cli command words:\n\nopenstack bgp speaker peer association list SPEAKER\nopenstack bgp speaker peer association show SPEAKER ASSOCIATION-NAME-OR-ID\nopenstack bgp speaker peer association create SPEAKER ASSOCIATION-NAME PEER (*)\nopenstack bgp speaker peer association delete SPEAKER ASSOCIATION-NAME-OR-ID\n\nThis clearly violates multiple user expectations and osc conventions:\n\n1) The user would expect the globally unique id of an association to be sufficient in identifying the association.\n2) osc only uses multiple positional parameters for nameless associations (like router add subnet).\n3) osc uses only one positional parameter for operations on named objects, which is always the name.\n*) This one is particularly confusing because we don\u0027t use \"associate\" in the verb form (bgp speaker associate peer), so we have speaker-peer-association order in the command words and speaker-association-peer order in the arguments. But this kind of depends on whether the user reads the command as \"bgp speaker peer-association\" or \"bgp speaker peer association\".\n\nDespite these problems I don\u0027t really see a better cli syntax. We may have an alternative (of which I don\u0027t know if it\u0027s better) if we completely drop the name attribute of the association (both client and server side). Then we could do this:\n\nopenstack bgp speaker peer association list SPEAKER\nopenstack bgp speaker peer association show SPEAKER [--association ASSOCIATION | --peer PEER]\nopenstack bgp speaker peer association create SPEAKER --peer PEER\nopenstack bgp speaker peer association delete SPEAKER [--association ASSOCIATION | --peer PEER]\n\nWhich could be simplified to:\n\nopenstack bgp speaker peer association list SPEAKER\nopenstack bgp speaker peer association show SPEAKER ASSOCIATION\nopenstack bgp speaker peer association create SPEAKER PEER\nopenstack bgp speaker peer association delete SPEAKER ASSOCIATION\n\nWhichever we do it may be worth to also compare the syntax to \u0027openstack network qos rule\u0027 operations.","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"77d9acc23113c7fce8bb2b2d923a47ae61065109","unresolved":true,"context_lines":[{"line_number":39,"context_line":"    def get_parser(self, prog_name):"},{"line_number":40,"context_line":"        parser \u003d super(CreateSpeakerPeerAssociation, self)\\"},{"line_number":41,"context_line":"                .get_parser(prog_name)"},{"line_number":42,"context_line":"        parser.add_argument("},{"line_number":43,"context_line":"            \u0027name\u0027,"},{"line_number":44,"context_line":"            metavar\u003d\u0027\u003cname\u003e\u0027,"},{"line_number":45,"context_line":"            help\u003d_(\"Name of the BGP speaker peer association to create\"))"},{"line_number":46,"context_line":"        parser.add_argument("},{"line_number":47,"context_line":"            \u0027bgp_speaker\u0027,"},{"line_number":48,"context_line":"            metavar\u003d\u0027BGP_SPEAKER\u0027,"},{"line_number":49,"context_line":"            help\u003d_(\u0027ID or name of the BGP speaker.\u0027))"},{"line_number":50,"context_line":"        parser.add_argument("},{"line_number":51,"context_line":"            \u0027bgp_peer\u0027,"},{"line_number":52,"context_line":"            metavar\u003d\u0027BGP_PEER\u0027,"},{"line_number":53,"context_line":"            help\u003d_(\u0027ID or name of the peer to add.\u0027))"},{"line_number":54,"context_line":"        nc_osc_utils.add_project_owner_option_to_parser(parser)"},{"line_number":55,"context_line":"        return parser"},{"line_number":56,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"a38c6895_c13b64f5","line":53,"range":{"start_line":42,"start_character":0,"end_line":53,"end_character":53},"in_reply_to":"cf64cb2c_480a9919","updated":"2021-10-26 12:16:30.000000000","message":"There are a few problems which I found for using association objects as a complete first class objects.\n1. Similar associations objects(router/peer) are used in networking-bgpvpn also. So it was not sure how to make the router/peer association unique for n-d-r.\n2. the url needs both speaker id and association id since speaker is parent of peer association. So, it was not clear whether any options were there to discard speaker-id while using it from client.\n\nAnd regarding the order of arguments, the suggestion of bringing speaker before association looks better and clear. \n\nIn the current patch , I used the approach followed in networking-bgpvpn (SPEAKER comes after ASSOCIATION-NAME-OR-ID). Not sure why this order is being used there. \n\nCan I go ahead with the below change?\nopenstack bgp speaker peer association list SPEAKER\nopenstack bgp speaker peer association show SPEAKER ASSOCIATION-NAME-OR-ID\nopenstack bgp speaker peer association create SPEAKER ASSOCIATION-NAME PEER (*)\nopenstack bgp speaker peer association delete SPEAKER ASSOCIATION-NAME-OR-ID\nMain difference from the current patch would be that SPEAKER comes before ASSOCIATION-NAME","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"3e74f39310ddf61ca056857b2c604c83fec14de1","unresolved":true,"context_lines":[{"line_number":68,"context_line":"        return display_columns, data"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"class DeleteSpeakerPeerAssociation(command.ShowOne):"},{"line_number":72,"context_line":"    _description \u003d _(\"Delete a BGP speaker peer association\")"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def get_parser(self, prog_name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"0ce62129_d01f9c3f","line":71,"updated":"2021-10-25 11:06:50.000000000","message":"\"delete\" works for me but there\u0027s some noise (\"cannot unpack ...\") printed during it:\n\n $ openstack bgp speaker peer association create speaker0-peer0 speaker0 peer0\n +------------+--------------------------------------+\n | Field      | Value                                |\n +------------+--------------------------------------+\n | id         | 6ab86fb2-8023-4995-bcc7-11edc423b0d8 |\n | name       | speaker0-peer0                       |\n | peer_id    | d8ccff6d-2aea-4996-b4c3-3dce202ac9a8 |\n | project_id | 39fbe5eb77c448a188a2ca42e9e4ed04     |\n | status     | DOWN                                 |\n | tenant_id  | 39fbe5eb77c448a188a2ca42e9e4ed04     |\n +------------+--------------------------------------+\n $ openstack bgp speaker peer association delete speaker0-peer0 speaker0\n cannot unpack non-iterable NoneType object","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"d655d82854ec7a280b31523d883c8be0affcf683","unresolved":true,"context_lines":[{"line_number":68,"context_line":"        return display_columns, data"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"class DeleteSpeakerPeerAssociation(command.ShowOne):"},{"line_number":72,"context_line":"    _description \u003d _(\"Delete a BGP speaker peer association\")"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def get_parser(self, prog_name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"417dc77c_52e268cd","line":71,"range":{"start_line":71,"start_character":43,"end_line":71,"end_character":50},"updated":"2021-10-25 14:30:10.000000000","message":"Is the base class intentionally ShowOne?","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"77d9acc23113c7fce8bb2b2d923a47ae61065109","unresolved":false,"context_lines":[{"line_number":68,"context_line":"        return display_columns, data"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"class DeleteSpeakerPeerAssociation(command.ShowOne):"},{"line_number":72,"context_line":"    _description \u003d _(\"Delete a BGP speaker peer association\")"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def get_parser(self, prog_name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"b4cc1114_b0e9fb3a","line":71,"in_reply_to":"0ce62129_d01f9c3f","updated":"2021-10-26 12:16:30.000000000","message":"The issue got resolved after changing to command.Command instead of command.ShowOne","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"77d9acc23113c7fce8bb2b2d923a47ae61065109","unresolved":false,"context_lines":[{"line_number":68,"context_line":"        return display_columns, data"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"class DeleteSpeakerPeerAssociation(command.ShowOne):"},{"line_number":72,"context_line":"    _description \u003d _(\"Delete a BGP speaker peer association\")"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def get_parser(self, prog_name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"c2235a76_564d76bb","line":71,"range":{"start_line":71,"start_character":43,"end_line":71,"end_character":50},"in_reply_to":"417dc77c_52e268cd","updated":"2021-10-26 12:16:30.000000000","message":"No, will change it to command.Command","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"4a90da3fa8f718bc72dfcfb259c26b189fb26a0f","unresolved":true,"context_lines":[{"line_number":114,"context_line":"        _speaker_id \u003d client.find_resource(constants.BGP_SPEAKER,"},{"line_number":115,"context_line":"                                           parsed_args.bgp_speaker)[\u0027id\u0027]"},{"line_number":116,"context_line":"        data \u003d client.list_bgp_speaker_peer_associations(_speaker_id)"},{"line_number":117,"context_line":"        headers \u003d (\u0027ID\u0027, \u0027Tenant ID\u0027, \u0027Peer\u0027, \u0027Status\u0027)"},{"line_number":118,"context_line":"        columns \u003d (\u0027id\u0027, \u0027tenant_id\u0027, \u0027peer_id\u0027, \u0027status\u0027)"},{"line_number":119,"context_line":"        return (headers, (disp_util.get_dict_properties(s, columns)"},{"line_number":120,"context_line":"                          for s in data[constants.BGP_PEER_ASSOCIATIONS]))"}],"source_content_type":"text/x-python","patch_set":3,"id":"d1ef86aa_1a41e2dc","line":117,"range":{"start_line":117,"start_character":26,"end_line":117,"end_character":35},"updated":"2021-10-25 11:00:15.000000000","message":"One more thing came to mind: In the past there was a rename from \"tenant\" to \"project\". The APIs usually have both tenant_id and project_id to stay backward compatible. But at new places I believe we should use project_id and use Project instead of Tenant.","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"8fb0384252250b09f7473440c1d40672f70b6246","unresolved":true,"context_lines":[{"line_number":114,"context_line":"        _speaker_id \u003d client.find_resource(constants.BGP_SPEAKER,"},{"line_number":115,"context_line":"                                           parsed_args.bgp_speaker)[\u0027id\u0027]"},{"line_number":116,"context_line":"        data \u003d client.list_bgp_speaker_peer_associations(_speaker_id)"},{"line_number":117,"context_line":"        headers \u003d (\u0027ID\u0027, \u0027Tenant ID\u0027, \u0027Peer\u0027, \u0027Status\u0027)"},{"line_number":118,"context_line":"        columns \u003d (\u0027id\u0027, \u0027tenant_id\u0027, \u0027peer_id\u0027, \u0027status\u0027)"},{"line_number":119,"context_line":"        return (headers, (disp_util.get_dict_properties(s, columns)"},{"line_number":120,"context_line":"                          for s in data[constants.BGP_PEER_ASSOCIATIONS]))"}],"source_content_type":"text/x-python","patch_set":3,"id":"9927884b_b1eae556","line":117,"updated":"2021-10-25 11:02:36.000000000","message":"Plus if we have names for the associations, let\u0027s show them here.","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"77d9acc23113c7fce8bb2b2d923a47ae61065109","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        _speaker_id \u003d client.find_resource(constants.BGP_SPEAKER,"},{"line_number":115,"context_line":"                                           parsed_args.bgp_speaker)[\u0027id\u0027]"},{"line_number":116,"context_line":"        data \u003d client.list_bgp_speaker_peer_associations(_speaker_id)"},{"line_number":117,"context_line":"        headers \u003d (\u0027ID\u0027, \u0027Tenant ID\u0027, \u0027Peer\u0027, \u0027Status\u0027)"},{"line_number":118,"context_line":"        columns \u003d (\u0027id\u0027, \u0027tenant_id\u0027, \u0027peer_id\u0027, \u0027status\u0027)"},{"line_number":119,"context_line":"        return (headers, (disp_util.get_dict_properties(s, columns)"},{"line_number":120,"context_line":"                          for s in data[constants.BGP_PEER_ASSOCIATIONS]))"}],"source_content_type":"text/x-python","patch_set":3,"id":"93ff1c23_bc6396f5","line":117,"in_reply_to":"9927884b_b1eae556","updated":"2021-10-26 12:16:30.000000000","message":"Done","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"77d9acc23113c7fce8bb2b2d923a47ae61065109","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        _speaker_id \u003d client.find_resource(constants.BGP_SPEAKER,"},{"line_number":115,"context_line":"                                           parsed_args.bgp_speaker)[\u0027id\u0027]"},{"line_number":116,"context_line":"        data \u003d client.list_bgp_speaker_peer_associations(_speaker_id)"},{"line_number":117,"context_line":"        headers \u003d (\u0027ID\u0027, \u0027Tenant ID\u0027, \u0027Peer\u0027, \u0027Status\u0027)"},{"line_number":118,"context_line":"        columns \u003d (\u0027id\u0027, \u0027tenant_id\u0027, \u0027peer_id\u0027, \u0027status\u0027)"},{"line_number":119,"context_line":"        return (headers, (disp_util.get_dict_properties(s, columns)"},{"line_number":120,"context_line":"                          for s in data[constants.BGP_PEER_ASSOCIATIONS]))"}],"source_content_type":"text/x-python","patch_set":3,"id":"09ec6952_58b63ed4","line":117,"range":{"start_line":117,"start_character":26,"end_line":117,"end_character":35},"in_reply_to":"d1ef86aa_1a41e2dc","updated":"2021-10-26 12:16:30.000000000","message":"Done","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"working for Ericsson, UTC+1 (+DST)"},"change_message_id":"cd4fd6377b4751480ae2033e922fcceea3f88179","unresolved":true,"context_lines":[{"line_number":129,"context_line":"        parser.add_argument("},{"line_number":130,"context_line":"            \u0027peer_association\u0027,"},{"line_number":131,"context_line":"            metavar\u003d\u0027PEER_ASSOCIATION\u0027,"},{"line_number":132,"context_line":"            help\u003d_(\u0027ID or name of the peer association to remove.\u0027))"},{"line_number":133,"context_line":"        parser.add_argument("},{"line_number":134,"context_line":"            \u0027bgp_speaker\u0027,"},{"line_number":135,"context_line":"            metavar\u003d\u0027BGP_SPEAKER\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"fbab6882_3e608b38","line":132,"range":{"start_line":132,"start_character":58,"end_line":132,"end_character":64},"updated":"2021-10-20 12:43:43.000000000","message":"copy-paste error","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"},{"author":{"_account_id":33273,"name":"Manu B","display_name":"Manu B","email":"manu.b@est.tech","username":"eceghkl"},"change_message_id":"77d9acc23113c7fce8bb2b2d923a47ae61065109","unresolved":false,"context_lines":[{"line_number":129,"context_line":"        parser.add_argument("},{"line_number":130,"context_line":"            \u0027peer_association\u0027,"},{"line_number":131,"context_line":"            metavar\u003d\u0027PEER_ASSOCIATION\u0027,"},{"line_number":132,"context_line":"            help\u003d_(\u0027ID or name of the peer association to remove.\u0027))"},{"line_number":133,"context_line":"        parser.add_argument("},{"line_number":134,"context_line":"            \u0027bgp_speaker\u0027,"},{"line_number":135,"context_line":"            metavar\u003d\u0027BGP_SPEAKER\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"1deb7efb_6aa889bb","line":132,"range":{"start_line":132,"start_character":58,"end_line":132,"end_character":64},"in_reply_to":"fbab6882_3e608b38","updated":"2021-10-26 12:16:30.000000000","message":"Done","commit_id":"5663d42cfd51207fa0e3e71a4d785fcfcc8b0c20"}]}