)]}'
{"neutron/services/qos/qos_plugin.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cc524c3c0273c4d396975d492f570aa5a8f2c5d3","unresolved":false,"context_lines":[{"line_number":433,"context_line":"            rule_to_update \u003d policy.get_rule_by_id(rule_id)"},{"line_number":434,"context_line":"            rule \u003d rule_cls(context, id\u003drule_id)"},{"line_number":435,"context_line":""},{"line_number":436,"context_line":"            # Note(lajoskatona): For QoS rules duplicates_compare_fields is a"},{"line_number":437,"context_line":"            # list of fields that is used to check whether the result of the"},{"line_number":438,"context_line":"            # update could be a duplicate"},{"line_number":439,"context_line":"            # (see: qos_policy_validator.check_rules_conflict)."}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_61096629","line":436,"updated":"2019-02-26 14:52:16.000000000","message":"This is just a personal opinion: we need to avoid these long notes. Code should be self-explanatory.","commit_id":"5609c0160e2c66fa90ea59e8e5ede399b33bb8ec"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"54fc2ee43b1d1359d592db06b8a026944661cceb","unresolved":false,"context_lines":[{"line_number":433,"context_line":"            rule_to_update \u003d policy.get_rule_by_id(rule_id)"},{"line_number":434,"context_line":"            rule \u003d rule_cls(context, id\u003drule_id)"},{"line_number":435,"context_line":""},{"line_number":436,"context_line":"            # Note(lajoskatona): For QoS rules duplicates_compare_fields is a"},{"line_number":437,"context_line":"            # list of fields that is used to check whether the result of the"},{"line_number":438,"context_line":"            # update could be a duplicate"},{"line_number":439,"context_line":"            # (see: qos_policy_validator.check_rules_conflict)."}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_8a09e82b","line":436,"in_reply_to":"9fdfeff1_61096629","updated":"2019-02-27 14:07:13.000000000","message":"I shortened my novel, and moved to the _add_missing_duplicite_compare_fields method","commit_id":"5609c0160e2c66fa90ea59e8e5ede399b33bb8ec"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"cc524c3c0273c4d396975d492f570aa5a8f2c5d3","unresolved":false,"context_lines":[{"line_number":441,"context_line":"            # field the above checker will raise AttributeError. To avoid this"},{"line_number":442,"context_line":"            # if any of these mandatory fields are not in the update"},{"line_number":443,"context_line":"            # request, add them to rule_data."},{"line_number":444,"context_line":"            self._fix_mandatory_update_fields(rule_data, rule_to_update)"},{"line_number":445,"context_line":""},{"line_number":446,"context_line":"            rule.update_fields(rule_data, reset_changes\u003dTrue)"},{"line_number":447,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_01973a92","line":444,"updated":"2019-02-26 14:52:16.000000000","message":"1) Instead of creating this new function, I suggest to modify check_rules_conflict, by passing \"rule_to_update\" to this function. Inside it, do something like:\n\nfor field in (field for field in original_rule.duplicates_compare_fields if\n              field not in rule_obj)\n    rule_obj[field] \u003d rule_to_update[field]\n\nDoing this you don\u0027t modify \"rule_data\" before using it and everything is solved in two lines.\n\n2) \"_fix_mandatory_update_fields\" is not a good name. It\u0027s not mandatory to always update \"direction\".","commit_id":"5609c0160e2c66fa90ea59e8e5ede399b33bb8ec"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"54fc2ee43b1d1359d592db06b8a026944661cceb","unresolved":false,"context_lines":[{"line_number":441,"context_line":"            # field the above checker will raise AttributeError. To avoid this"},{"line_number":442,"context_line":"            # if any of these mandatory fields are not in the update"},{"line_number":443,"context_line":"            # request, add them to rule_data."},{"line_number":444,"context_line":"            self._fix_mandatory_update_fields(rule_data, rule_to_update)"},{"line_number":445,"context_line":""},{"line_number":446,"context_line":"            rule.update_fields(rule_data, reset_changes\u003dTrue)"},{"line_number":447,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_2a979c92","line":444,"in_reply_to":"9fdfeff1_01973a92","updated":"2019-02-27 14:07:13.000000000","message":"1) create_policy_rule uses the same check_rules_conflict, so if I go that way I have to add condition to check_rules_conflict or call from create_policy_rules like this: check_rules_conflict(policy, rule, rule)\n\n2) I changed the name to _add_missing_duplicite_compare_fields","commit_id":"5609c0160e2c66fa90ea59e8e5ede399b33bb8ec"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"532b3c0564cb829ec1b19c7db7416261d49dc61a","unresolved":false,"context_lines":[{"line_number":441,"context_line":"            # field the above checker will raise AttributeError. To avoid this"},{"line_number":442,"context_line":"            # if any of these mandatory fields are not in the update"},{"line_number":443,"context_line":"            # request, add them to rule_data."},{"line_number":444,"context_line":"            self._fix_mandatory_update_fields(rule_data, rule_to_update)"},{"line_number":445,"context_line":""},{"line_number":446,"context_line":"            rule.update_fields(rule_data, reset_changes\u003dTrue)"},{"line_number":447,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_8a57a801","line":444,"in_reply_to":"9fdfeff1_01973a92","updated":"2019-02-27 14:04:18.000000000","message":"Ok, so you don\u0027t find this suggestion (two code lines) better than adding a new function.","commit_id":"5609c0160e2c66fa90ea59e8e5ede399b33bb8ec"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"64ccf12bb36574b6aa71f2e296b9597327e0011b","unresolved":false,"context_lines":[{"line_number":441,"context_line":"            # field the above checker will raise AttributeError. To avoid this"},{"line_number":442,"context_line":"            # if any of these mandatory fields are not in the update"},{"line_number":443,"context_line":"            # request, add them to rule_data."},{"line_number":444,"context_line":"            self._fix_mandatory_update_fields(rule_data, rule_to_update)"},{"line_number":445,"context_line":""},{"line_number":446,"context_line":"            rule.update_fields(rule_data, reset_changes\u003dTrue)"},{"line_number":447,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_81b51def","line":444,"in_reply_to":"9fdfeff1_1c78bdbe","updated":"2019-03-01 06:15:43.000000000","message":"Thanks Rodolfo, let\u0027s see others opinion.","commit_id":"5609c0160e2c66fa90ea59e8e5ede399b33bb8ec"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"286256d7bbc08d467324b4e4acb10361ba8960a5","unresolved":false,"context_lines":[{"line_number":441,"context_line":"            # field the above checker will raise AttributeError. To avoid this"},{"line_number":442,"context_line":"            # if any of these mandatory fields are not in the update"},{"line_number":443,"context_line":"            # request, add them to rule_data."},{"line_number":444,"context_line":"            self._fix_mandatory_update_fields(rule_data, rule_to_update)"},{"line_number":445,"context_line":""},{"line_number":446,"context_line":"            rule.update_fields(rule_data, reset_changes\u003dTrue)"},{"line_number":447,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_1c78bdbe","line":444,"in_reply_to":"9fdfeff1_4a47801e","updated":"2019-02-28 18:05:11.000000000","message":"I understand your point. I would prefer someone else to review this patch.\n\nBut as I said: if you introduce a keyword argument (default to None) in \"check_rules_conflict\" and only here, in \"update_policy_rule\", you add \"rule_to_update\" as value, using those two lines you can add the needed parameters. The point is to minimize the length of the code to make it easier and faster to review. Adding a new function here, IMO, is not needed.\n\nBut as I said, other reviewers are welcome.","commit_id":"5609c0160e2c66fa90ea59e8e5ede399b33bb8ec"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"7c79b44e389d6fd5bcb6af071fd796db8a79f6de","unresolved":false,"context_lines":[{"line_number":441,"context_line":"            # field the above checker will raise AttributeError. To avoid this"},{"line_number":442,"context_line":"            # if any of these mandatory fields are not in the update"},{"line_number":443,"context_line":"            # request, add them to rule_data."},{"line_number":444,"context_line":"            self._fix_mandatory_update_fields(rule_data, rule_to_update)"},{"line_number":445,"context_line":""},{"line_number":446,"context_line":"            rule.update_fields(rule_data, reset_changes\u003dTrue)"},{"line_number":447,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9fdfeff1_4a47801e","line":444,"in_reply_to":"9fdfeff1_8a57a801","updated":"2019-02-27 14:10:26.000000000","message":"My concern is that the same method is used by create as well and I would like to avoid touching something that is not affected by the bug","commit_id":"5609c0160e2c66fa90ea59e8e5ede399b33bb8ec"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"a87e7c32e57264a5d212dd6f3542404ecc511914","unresolved":false,"context_lines":[{"line_number":396,"context_line":""},{"line_number":397,"context_line":"        return rule"},{"line_number":398,"context_line":""},{"line_number":399,"context_line":"    def _add_missing_duplicite_compare_fields(self, updated_fields,"},{"line_number":400,"context_line":"                                              original_rule):"},{"line_number":401,"context_line":"        # Note(lajoskatona): For QoS rules duplicates_compare_fields is a"},{"line_number":402,"context_line":"        # list of fields that is used to check whether the result of the"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_a88da131","line":399,"range":{"start_line":399,"start_character":27,"end_line":399,"end_character":28},"updated":"2019-03-01 10:27:28.000000000","message":"That looks like a typo. I guess if you want to refer to the other method then it\u0027s duplicAteS_compare_fields.","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"a87e7c32e57264a5d212dd6f3542404ecc511914","unresolved":false,"context_lines":[{"line_number":405,"context_line":"            if field not in updated_fields:"},{"line_number":406,"context_line":"                LOG.debug(\u0027Mandatory field %s  was not in the update request, \u0027"},{"line_number":407,"context_line":"                          \u0027fetch it from the updated rule\u0027, field)"},{"line_number":408,"context_line":"                updated_fields[field] \u003d getattr(original_rule, field, None)"},{"line_number":409,"context_line":""},{"line_number":410,"context_line":"    @db_base_plugin_common.convert_result_to_dict"},{"line_number":411,"context_line":"    def update_policy_rule(self, context, rule_cls, rule_id, policy_id,"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_48fc3571","line":408,"range":{"start_line":408,"start_character":70,"end_line":408,"end_character":74},"updated":"2019-03-01 10:27:28.000000000","message":"Are you sure you want a add a None-valued field to the update when that field was neither present in the update nor in the original rule? I\u0027m not sure for what input could this happen, but it sounds safer to skip a field like that.","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"a2286a5b708d99b0a5a05eea3f21fe2f3b65dfe1","unresolved":false,"context_lines":[{"line_number":434,"context_line":"            # Ensure the rule belongs to the policy."},{"line_number":435,"context_line":"            checker.check_bandwidth_rule_conflict(policy, rule_data)"},{"line_number":436,"context_line":"            rule_to_update \u003d policy.get_rule_by_id(rule_id)"},{"line_number":437,"context_line":"            rule \u003d rule_cls(context, id\u003drule_id)"},{"line_number":438,"context_line":""},{"line_number":439,"context_line":"            self._add_missing_duplicite_compare_fields(rule_data,"},{"line_number":440,"context_line":"                                                       rule_to_update)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_a616f248","line":437,"range":{"start_line":437,"start_character":12,"end_line":437,"end_character":48},"updated":"2019-03-05 10:43:55.000000000","message":"About PS3 (but I need to point to this code): when a rule is updated, \"direction\" can be provided or not./\n\nIn case \"direction\" is not provided, this line will create a \"rule_cls\" rule object with \"direction\" set to the default value, \"egress\". In case the rule to be updated (rule_id) is an ingress one, rule.direction \u003d egress in any case.\n\nIf \"rule_data\" does not update this parameter, rule.direction will still be \"egress\".\n\nPlease, see log: [1]\n\nActually this is bug in the current code.\n\nWhat I don\u0027t understand is why we are doing L436-437. We can solve the bug and your request by doing:\n\n  rule \u003d policy.get_rule_by_id(rule_id)  # \"rule\" is the object\n                                         # to be updated.\n\n[1] http://paste.openstack.org/show/747278/","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"d33881f9a58042554b01d61ce5b2d99b9a6b181c","unresolved":false,"context_lines":[{"line_number":434,"context_line":"            # Ensure the rule belongs to the policy."},{"line_number":435,"context_line":"            checker.check_bandwidth_rule_conflict(policy, rule_data)"},{"line_number":436,"context_line":"            rule_to_update \u003d policy.get_rule_by_id(rule_id)"},{"line_number":437,"context_line":"            rule \u003d rule_cls(context, id\u003drule_id)"},{"line_number":438,"context_line":""},{"line_number":439,"context_line":"            self._add_missing_duplicite_compare_fields(rule_data,"},{"line_number":440,"context_line":"                                                       rule_to_update)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_8a9431ea","line":437,"range":{"start_line":437,"start_character":12,"end_line":437,"end_character":48},"in_reply_to":"5fc1f717_ad8efdcd","updated":"2019-03-06 13:04:24.000000000","message":"Done\nI open a bug for the default direction of bandwidth_limit rule as discussed","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"e33f78416cf2851daf8cb66d92fb65519bf7b684","unresolved":false,"context_lines":[{"line_number":434,"context_line":"            # Ensure the rule belongs to the policy."},{"line_number":435,"context_line":"            checker.check_bandwidth_rule_conflict(policy, rule_data)"},{"line_number":436,"context_line":"            rule_to_update \u003d policy.get_rule_by_id(rule_id)"},{"line_number":437,"context_line":"            rule \u003d rule_cls(context, id\u003drule_id)"},{"line_number":438,"context_line":""},{"line_number":439,"context_line":"            self._add_missing_duplicite_compare_fields(rule_data,"},{"line_number":440,"context_line":"                                                       rule_to_update)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_ad8efdcd","line":437,"range":{"start_line":437,"start_character":12,"end_line":437,"end_character":48},"in_reply_to":"9fdfeff1_3f33dc15","updated":"2019-03-05 17:48:45.000000000","message":"By making the change commented [1], you solve:\n- The current bug we have when updating max-bw rules.\n- Your bug\n\n[1] rule \u003d policy.get_rule_by_id(rule_id)\n\nIn order to make max-bw similar to min-bw (forcing the user to define a direction during the creation), you can open another bug. This is not a priority and can be handled after this change.","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"a3add84e3a382c2307a0bc8749172ba14ec2ad2b","unresolved":false,"context_lines":[{"line_number":434,"context_line":"            # Ensure the rule belongs to the policy."},{"line_number":435,"context_line":"            checker.check_bandwidth_rule_conflict(policy, rule_data)"},{"line_number":436,"context_line":"            rule_to_update \u003d policy.get_rule_by_id(rule_id)"},{"line_number":437,"context_line":"            rule \u003d rule_cls(context, id\u003drule_id)"},{"line_number":438,"context_line":""},{"line_number":439,"context_line":"            self._add_missing_duplicite_compare_fields(rule_data,"},{"line_number":440,"context_line":"                                                       rule_to_update)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_3f33dc15","line":437,"range":{"start_line":437,"start_character":12,"end_line":437,"end_character":48},"in_reply_to":"9fdfeff1_77f0b292","updated":"2019-03-05 17:04:10.000000000","message":"I think option 2 is out of question since it would change the API.\n\nIt seems to me we have two bugs here. If that\u0027s right, I find option 3 the cleaner way to work.","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"bf6644c8e364937355ab865f602c5662c3447403","unresolved":false,"context_lines":[{"line_number":434,"context_line":"            # Ensure the rule belongs to the policy."},{"line_number":435,"context_line":"            checker.check_bandwidth_rule_conflict(policy, rule_data)"},{"line_number":436,"context_line":"            rule_to_update \u003d policy.get_rule_by_id(rule_id)"},{"line_number":437,"context_line":"            rule \u003d rule_cls(context, id\u003drule_id)"},{"line_number":438,"context_line":""},{"line_number":439,"context_line":"            self._add_missing_duplicite_compare_fields(rule_data,"},{"line_number":440,"context_line":"                                                       rule_to_update)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_77f0b292","line":437,"range":{"start_line":437,"start_character":12,"end_line":437,"end_character":48},"in_reply_to":"9fdfeff1_a616f248","updated":"2019-03-05 14:24:17.000000000","message":"Thanks Rodolfo, I badly assumed that minimum_bandwidth rule and bandwidth_limit rule works the same way.\n\nThe default egress is true only for bandwidth limit rule (see: https://github.com/openstack/neutron/blob/master/neutron/objects/qos/rule.py#L130), and in my mind that is a question now as I see your comment.\nSo as I see there\u0027s 3 way:\n\n1.) keep the way that bandwidth_limit rules have a default egress direction, and fix both rule type updates here, as you suggest.\n\n2.) remove the default egress direction for bandwidth_limit rule as an addition to PS3. That means that both bandwidth rule types will behave the same way, and we change the user experience, as for creating bandwidth limit now the user experience is that by default it will be an egress rule.\n\n3.) Change the scope of this patch to limit to minimum_bandwidth rule, and open a new bug for bandwidth_limit rule, and discuss that separately.","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"},{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"a87e7c32e57264a5d212dd6f3542404ecc511914","unresolved":false,"context_lines":[{"line_number":440,"context_line":"                                                       rule_to_update)"},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"            rule.update_fields(rule_data, reset_changes\u003dTrue)"},{"line_number":443,"context_line":""},{"line_number":444,"context_line":"            checker.check_rules_conflict(policy, rule)"},{"line_number":445,"context_line":"            rule.update()"},{"line_number":446,"context_line":"            policy.obj_load_attr(\u0027rules\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_48951531","line":443,"updated":"2019-03-01 10:27:28.000000000","message":"random whitespace","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"}],"neutron/tests/unit/services/qos/test_qos_plugin.py":[{"author":{"_account_id":15554,"name":"Bence Romsics","email":"bence.romsics@gmail.com","username":"ebenrom","status":"inactive contributor"},"change_message_id":"a87e7c32e57264a5d212dd6f3542404ecc511914","unresolved":false,"context_lines":[{"line_number":89,"context_line":"            \u0027minimum_bandwidth_rule\u0027: {"},{"line_number":90,"context_line":"                \u0027id\u0027: uuidutils.generate_uuid(),"},{"line_number":91,"context_line":"                \u0027min_kbps\u0027: 10,"},{"line_number":92,"context_line":"                \u0027direction\u0027: \u0027ingress\u0027}}"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"        self.policy \u003d policy_object.QosPolicy("},{"line_number":95,"context_line":"            self.ctxt, **self.policy_data[\u0027policy\u0027])"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fdfeff1_ee5ad11b","line":92,"updated":"2019-03-01 10:27:28.000000000","message":"Do we have a test that *does not* send a direction field? If not I think it would be a good idea to add one. So we have a regression test for the original bug condition.","commit_id":"7c18773bb1518a5fc572b1229ff27e52f3b15e8f"}]}