)]}'
{"ovsdbapp/schema/ovn_northbound/api.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"c5d5dfb2430821fd158076e05c3f299a5a28453c","unresolved":false,"context_lines":[{"line_number":147,"context_line":"    @abc.abstractmethod"},{"line_number":148,"context_line":"    def pg_acl_add(self, port_group, direction, priority, match, action,"},{"line_number":149,"context_line":"                   log\u003dFalse, may_exist\u003dFalse, severity\u003dNone, name\u003dNone,"},{"line_number":150,"context_line":"                   meter\u003dNone, **external_ids):"},{"line_number":151,"context_line":"        \"\"\"Add an ACL to \u0027port_group\u0027"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"        :param port_group:   The name or uuid of the port group"}],"source_content_type":"text/x-python","patch_set":4,"id":"e19d4e6f_2e8f5006","line":150,"range":{"start_line":150,"start_character":31,"end_line":150,"end_character":45},"updated":"2021-01-07 17:32:44.000000000","message":"Do you think that doing **columns here, even though there aren\u0027t currently any other columns, would be a good choice in case the table is expanded with some optional columns? Then just passing external_ids\u003d{\u0027a\u0027: \u0027b\u0027, \u0027x\u0027: \u0027y\u0027} etc.?\n\nI just noticed that the api.py definition of acl_add differs from the impl_idl.py version. The impl  has **external_ids and the api doesn\u0027t. Ugh. So you can probably ignore the previous paragraph since the API itself has been kind of silly since I added it. 😞 The shame.","commit_id":"f52d261ba2a57fe7807ef2708cd26ceee35bc033"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"97484328a6b4cceaf9769497d40b1c7761618e7a","unresolved":false,"context_lines":[{"line_number":147,"context_line":"    @abc.abstractmethod"},{"line_number":148,"context_line":"    def pg_acl_add(self, port_group, direction, priority, match, action,"},{"line_number":149,"context_line":"                   log\u003dFalse, may_exist\u003dFalse, severity\u003dNone, name\u003dNone,"},{"line_number":150,"context_line":"                   meter\u003dNone, **external_ids):"},{"line_number":151,"context_line":"        \"\"\"Add an ACL to \u0027port_group\u0027"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"        :param port_group:   The name or uuid of the port group"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba73c176_473fb9ac","line":150,"range":{"start_line":150,"start_character":31,"end_line":150,"end_character":45},"in_reply_to":"e19d4e6f_2e8f5006","updated":"2021-01-07 23:13:36.000000000","message":"lol. no worries!","commit_id":"f52d261ba2a57fe7807ef2708cd26ceee35bc033"}],"ovsdbapp/schema/ovn_northbound/commands.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"c5d5dfb2430821fd158076e05c3f299a5a28453c","unresolved":true,"context_lines":[{"line_number":1507,"context_line":"        self.columns \u003d columns"},{"line_number":1508,"context_line":""},{"line_number":1509,"context_line":"    def run_idl(self, txn):"},{"line_number":1510,"context_line":"        if self.may_exist:"},{"line_number":1511,"context_line":"            try:"},{"line_number":1512,"context_line":"                meter \u003d self.api.lookup(self.table_name, self.name)"},{"line_number":1513,"context_line":"                self.result \u003d rowview.RowView(meter)"}],"source_content_type":"text/x-python","patch_set":4,"id":"aff48e21_23905879","line":1510,"updated":"2021-01-07 17:32:44.000000000","message":"Let\u0027s say may_exist\u003dFalse and we have an existing row. That means we must fail. Unless I am misreading, it looks like this will try to create a new meter in that case? So it\u0027s relying on actually sending the transaction to ovsdb-server and letting it reject it due to a constraint violation?\n\nAt least traditionally, the idl C code (and therefor the original ovsdbapp code) would try to avoid sending transactions that it knew were broken w/ may_exist. It\u0027s annoying, because it means supporting may_exist means that we always have to do a lookup, even if we would otherwise just be creating an object. In general, I default to whatever ovn-nbctl does, which looks like it locally does the check--but it also doesn\u0027t look like it even supports may_exist. Maybe we don\u0027t need it then? Or does ovn-nbctl need to be updated to support it?","commit_id":"f52d261ba2a57fe7807ef2708cd26ceee35bc033"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"97484328a6b4cceaf9769497d40b1c7761618e7a","unresolved":false,"context_lines":[{"line_number":1507,"context_line":"        self.columns \u003d columns"},{"line_number":1508,"context_line":""},{"line_number":1509,"context_line":"    def run_idl(self, txn):"},{"line_number":1510,"context_line":"        if self.may_exist:"},{"line_number":1511,"context_line":"            try:"},{"line_number":1512,"context_line":"                meter \u003d self.api.lookup(self.table_name, self.name)"},{"line_number":1513,"context_line":"                self.result \u003d rowview.RowView(meter)"}],"source_content_type":"text/x-python","patch_set":4,"id":"97219e43_1d213121","line":1510,"in_reply_to":"aff48e21_23905879","updated":"2021-01-07 23:13:36.000000000","message":"TBH, I was using \u0027HAChassisGroupAddCommand\u0027 code (above) as a template on how we wanted to handle may_exist. I understand the intent to do whatever ovn-nbctl does, but thought of making\nmaking it a little nicer. ;^)\n\nYou are right that nbctl_meter_add currently does not have --may-exist:\nhttps://github.com/ovn-org/ovn/blob/f35e33228f6fec97bbecc1f01e28e366d826b585/utilities/ovn-nbctl.c#L2740-L2802","commit_id":"f52d261ba2a57fe7807ef2708cd26ceee35bc033"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"97484328a6b4cceaf9769497d40b1c7761618e7a","unresolved":true,"context_lines":[{"line_number":1524,"context_line":"        meter.name \u003d self.name"},{"line_number":1525,"context_line":"        meter.unit \u003d self.unit"},{"line_number":1526,"context_line":"        meter.bands \u003d [meter_band.uuid]"},{"line_number":1527,"context_line":"        meter.fair \u003d self.fair"},{"line_number":1528,"context_line":"        self.set_columns(meter, **self.columns)"},{"line_number":1529,"context_line":"        self.result \u003d meter.uuid"},{"line_number":1530,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"38083dd6_233e6081","line":1527,"range":{"start_line":1527,"start_character":8,"end_line":1527,"end_character":30},"updated":"2021-01-07 23:13:36.000000000","message":"Note: In next rev, I am adding a conditional here to not mess with \u0027fair\u0027, if that is not a column in the existing schema. Please let me know if that is dumb. 😊","commit_id":"f52d261ba2a57fe7807ef2708cd26ceee35bc033"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"c5d5dfb2430821fd158076e05c3f299a5a28453c","unresolved":true,"context_lines":[{"line_number":1537,"context_line":""},{"line_number":1538,"context_line":"    def run_idl(self, _txn):"},{"line_number":1539,"context_line":"        try:"},{"line_number":1540,"context_line":"            meter \u003d self.api.lookup(\u0027Meter\u0027, self.meter)"},{"line_number":1541,"context_line":"            meter.delete()"},{"line_number":1542,"context_line":"        except idlutils.RowNotFound as e:"},{"line_number":1543,"context_line":"            if self.if_exists:"}],"source_content_type":"text/x-python","patch_set":4,"id":"77edf0c1_78d25c9f","line":1540,"updated":"2021-01-07 17:32:44.000000000","message":"It looks like ovn-nbctl meter-del deletes all meters if a name isn\u0027t passed. Although we try to default to matching ovn-nbctl as exactly as possible api-wise, I\u0027m sure there are other places where we don\u0027t. If you don\u0027t think we should match that, I trust your judgement. Just pointing out the difference.\n\nI actually disagree with some choices in the ovn-nbctl code. Like it looks like it is impossible to actually look up a meter by its uuid, just it\u0027s name field. I\u0027ve noticed that in some other ovn-nbctl code as well, which is a departure from how ovs-vsctl did things. Maybe it is a good change as looking up \"by name or id\" can be ambiguous--but it is a change between the two.","commit_id":"f52d261ba2a57fe7807ef2708cd26ceee35bc033"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"97484328a6b4cceaf9769497d40b1c7761618e7a","unresolved":false,"context_lines":[{"line_number":1537,"context_line":""},{"line_number":1538,"context_line":"    def run_idl(self, _txn):"},{"line_number":1539,"context_line":"        try:"},{"line_number":1540,"context_line":"            meter \u003d self.api.lookup(\u0027Meter\u0027, self.meter)"},{"line_number":1541,"context_line":"            meter.delete()"},{"line_number":1542,"context_line":"        except idlutils.RowNotFound as e:"},{"line_number":1543,"context_line":"            if self.if_exists:"}],"source_content_type":"text/x-python","patch_set":4,"id":"52d4ed47_87797a6c","line":1540,"in_reply_to":"77edf0c1_78d25c9f","updated":"2021-01-07 23:13:36.000000000","message":"Good points!\n\nI find it both dangerous and unnecessary to use the empty name as a way do delete all meters. Just can\u0027t think of a good use case. If you do not mind, let\u0027s keep it this way. Note that we have a \u0027meter_list\u0027 in the api, so it should be really simple to have a loop that removes all, if that is really what user wants.\n\nYeah, the lookup by uuid or name seem to me to be a common thing, agree? So once again I an leaning towards common usage vs a verbatim of what ovn-nbctl provides. I hope I do not live to regret that. ;^)","commit_id":"f52d261ba2a57fe7807ef2708cd26ceee35bc033"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"d4f6618b95b54a39fc094eee2b048a88e6314169","unresolved":true,"context_lines":[{"line_number":1512,"context_line":"            self.result \u003d rowview.RowView(meter)"},{"line_number":1513,"context_line":"            if self.may_exist:"},{"line_number":1514,"context_line":"                return"},{"line_number":1515,"context_line":"            raise RuntimeError(\"Meter %s exists\" % self.name)"},{"line_number":1516,"context_line":"        except idlutils.RowNotFound:"},{"line_number":1517,"context_line":"            pass"},{"line_number":1518,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"8149b496_84391693","line":1515,"range":{"start_line":1515,"start_character":41,"end_line":1515,"end_character":47},"updated":"2021-01-13 09:36:03.000000000","message":"just a nitty nit: wouldn\u0027t be better to write \"already exists\"?","commit_id":"e2f782cd727506de3dbc2a32cc49920b0c792c0f"}]}