)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"d72ebcdce9c043f4d48a2420a1711a3af61ba829","unresolved":false,"context_lines":[{"line_number":20,"context_line":"blank and have functional justification for being blank (e.g. \"router\")"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Closes-Bug: #1437695"},{"line_number":23,"context_line":"Doesn\u0027t break the fix for bug: #1257467"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"Change-Id: I0ac76e132c4bd86da39863674d4cf8a22dad7034"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9a1a9d01_de9c1e10","line":23,"updated":"2015-09-29 23:09:38.000000000","message":"Why not simply Related-Bug: #... as previous paragraph already explains this","commit_id":"cd7694c676732210b0c15290461bdf16b07a34e6"},{"author":{"_account_id":17120,"name":"Manjeet Singh Bhatia","email":"manjeet.s.bhatia@intel.com","username":"manjeets"},"change_message_id":"d082169f9f943c165e3b2e0b22430b6cefb3e7d1","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     David Edery \u003cdavid.edery@alcatel-lucent.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-09-30 17:02:22 +0300"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"enable specific extra_dhcp_opt to be left blank"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In some cases there is a need for a blank value of the \"router\" extra"},{"line_number":10,"context_line":"dhcp option. This fix addresses this issue by introducing a simple"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"9a1a9d01_ee7a6b76","line":7,"updated":"2015-09-30 17:58:12.000000000","message":"Enable specific extra_dhcp_opt to be left blank.\n\nsuggestion.","commit_id":"a064727073f3951b26c5caf272e7624e75309848"},{"author":{"_account_id":17120,"name":"Manjeet Singh Bhatia","email":"manjeet.s.bhatia@intel.com","username":"manjeets"},"change_message_id":"d082169f9f943c165e3b2e0b22430b6cefb3e7d1","unresolved":false,"context_lines":[{"line_number":11,"context_line":"mechanism in the code that allows specific extra dhcp options to be left"},{"line_number":12,"context_line":"blank by applying a different opt_value validation of the data received"},{"line_number":13,"context_line":"from the REST call and different validation on the value that will be"},{"line_number":14,"context_line":"written to the DB."},{"line_number":15,"context_line":"This fix also takes into consideration bug #1257467 which claims that"},{"line_number":16,"context_line":"in case of a blank \"server-ip-address\" option a segmentation fault"},{"line_number":17,"context_line":"occurs in dnsmasq. I did not check this claim with a newer dnsmasq"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"9a1a9d01_2e6143a9","line":14,"updated":"2015-09-30 17:58:12.000000000","message":"leave line space b/w two paragraphs for better readability.","commit_id":"a064727073f3951b26c5caf272e7624e75309848"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"bab9beab7683793b3bc80328e543a71da1b2c8c4","unresolved":false,"context_lines":[{"line_number":21,"context_line":"blank and have functional justification for being blank (e.g. \"router\")"},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"Closes-Bug: #1437695"},{"line_number":24,"context_line":"Related-Bug: #1257467"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Change-Id: I0ac76e132c4bd86da39863674d4cf8a22dad7034"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"7a2fa921_d52756c8","line":24,"updated":"2015-10-08 03:34:27.000000000","message":"APIImpact and DocImpact are required.","commit_id":"ebc7a02d033ae431683c4257134a1f36b4f1f370"}],"neutron/db/extradhcpopt_db.py":[{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"bab9beab7683793b3bc80328e543a71da1b2c8c4","unresolved":false,"context_lines":[{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def _is_valid_opt_value(self, opt_name, opt_value):"},{"line_number":64,"context_line":"        if opt_name in edo_ext.VALID_BLANK_EXTRA_DHCP_OPTS:"},{"line_number":65,"context_line":"            return opt_value is not None"},{"line_number":66,"context_line":"        return opt_value"},{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    def _process_port_create_extra_dhcp_opts(self, context, port,"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a2fa921_35700adc","line":65,"updated":"2015-10-08 03:34:27.000000000","message":"Looking at L.114, we need to pass \"None\" (null in JSON) to remove the option.\n\nAfter this patch, we can delete some dhcp option \nHowever, for example, opt_name \u0027router\u0027 has a value None and we can PUT  None for opt_name \u0027router\u0027 in an API request to delete opt-name \u0027router\u0027.\nIt looks a bit strange that pushing the same value will delete the option....\n\nA strange scenario:\n\n- POST [{\u0027opt_name: \u0027router\u0027, \u0027opt_value\u0027: None}] when creating a port\n- A port is created with \u0027router\u0027 dhcp opt.\n- PUT  [{\u0027opt_name: \u0027router\u0027, \u0027opt_value\u0027: None}] when update a port with the same value.\n- \u0027router\u0027 dhcp_opt for the port will be deleted.\n\nextra_dhcp_opts is a tricky point of the current Neutron API, so I am okay with this version, but I would like to see clear description in the document.\nAt least the new behavior cannot be understood without reading the code.","commit_id":"ebc7a02d033ae431683c4257134a1f36b4f1f370"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"f05da49a26c81f0945b03bd921a3cdb81a2ec3d2","unresolved":false,"context_lines":[{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def _is_valid_opt_value(self, opt_name, opt_value):"},{"line_number":64,"context_line":"        if opt_name in edo_ext.VALID_BLANK_EXTRA_DHCP_OPTS:"},{"line_number":65,"context_line":"            return opt_value is not None"},{"line_number":66,"context_line":"        return opt_value"},{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    def _process_port_create_extra_dhcp_opts(self, context, port,"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a2fa921_3ac46b8f","line":65,"in_reply_to":"7a2fa921_35700adc","updated":"2015-10-08 12:18:52.000000000","message":"My understanding was not correct. This method returns False when None is specified for opt_name in VALID_BLANK_EXTRA_DHCP_OPTS, so the option is not added to the database.","commit_id":"ebc7a02d033ae431683c4257134a1f36b4f1f370"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"f05da49a26c81f0945b03bd921a3cdb81a2ec3d2","unresolved":false,"context_lines":[{"line_number":114,"context_line":"                            if upd_rec[\u0027opt_value\u0027] is None:"},{"line_number":115,"context_line":"                                context.session.delete(opt)"},{"line_number":116,"context_line":"                            else:"},{"line_number":117,"context_line":"                                if opt[\u0027opt_value\u0027] !\u003d upd_rec[\u0027opt_value\u0027]:"},{"line_number":118,"context_line":"                                    opt.update("},{"line_number":119,"context_line":"                                        {\u0027opt_value\u0027: upd_rec[\u0027opt_value\u0027]})"},{"line_number":120,"context_line":"                            break"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a2fa921_db6a16ee","line":117,"updated":"2015-10-08 12:18:52.000000000","message":"Nit: If an empty string is specified in PUT operation,\nupd_rec[\u0027opt_value\u0027] is \"\" and opt[\u0027opt_value\u0027] is True,\nand the update option will be executed.","commit_id":"ebc7a02d033ae431683c4257134a1f36b4f1f370"},{"author":{"_account_id":14611,"name":"dasm","display_name":"dasm","email":"dsmigiel@redhat.com","username":"dasm"},"change_message_id":"95c78ece8c6e2838c098364f2880af9cf6476304","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        # is None or empty (though \"return opt_value\" is enough, it returns"},{"line_number":72,"context_line":"        # the value and not True/False. This is why i\u0027ve added the \"opt_value"},{"line_number":73,"context_line":"        # is not None before)"},{"line_number":74,"context_line":"        return opt_value is not None and opt_value"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    def _process_port_create_extra_dhcp_opts(self, context, port,"},{"line_number":77,"context_line":"                                             extra_dhcp_opts):"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a2fa921_d65d997a","line":74,"updated":"2015-10-12 09:44:06.000000000","message":"If you\u0027d like to have just True/False you need to add casting for boolean. In other case, you still have.\n\n \u003e\u003e\u003e opt_value \u003d \"test\"\n \u003e\u003e\u003e opt_value is not None and opt_value\n \u0027test\u0027\n \u003e\u003e\u003e bool(opt_value is not None and opt_value)\n True\n \u003e\u003e\u003e","commit_id":"0640e52d9303413419b545bdc03bfff102c9d060"},{"author":{"_account_id":15427,"name":"David Edery","email":"david.edery@gmail.com","username":"dedery"},"change_message_id":"1bfb078c3f1750ee11d6132f655aa0b7abdf75c8","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        # is None or empty (though \"return opt_value\" is enough, it returns"},{"line_number":72,"context_line":"        # the value and not True/False. This is why i\u0027ve added the \"opt_value"},{"line_number":73,"context_line":"        # is not None before)"},{"line_number":74,"context_line":"        return opt_value is not None and opt_value"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    def _process_port_create_extra_dhcp_opts(self, context, port,"},{"line_number":77,"context_line":"                                             extra_dhcp_opts):"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a2fa921_0c2b0035","line":74,"in_reply_to":"7a2fa921_d65d997a","updated":"2015-10-12 10:18:32.000000000","message":"Great! Thanks, will change it.","commit_id":"0640e52d9303413419b545bdc03bfff102c9d060"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"f8fdd6ada863e2b7faad0dc9c15c1c0459441366","unresolved":false,"context_lines":[{"line_number":123,"context_line":"                                if self._is_valid_opt_value("},{"line_number":124,"context_line":"                                        opt[\u0027opt_name\u0027],"},{"line_number":125,"context_line":"                                        upd_rec[\u0027opt_value\u0027]) and \\"},{"line_number":126,"context_line":"                                   (opt[\u0027opt_value\u0027] !\u003d upd_rec[\u0027opt_value\u0027]):"},{"line_number":127,"context_line":"                                    opt.update("},{"line_number":128,"context_line":"                                        {\u0027opt_value\u0027: upd_rec[\u0027opt_value\u0027]})"},{"line_number":129,"context_line":"                            break"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a2fa921_82cd9788","line":126,"updated":"2015-10-12 12:49:18.000000000","message":"I am not sure in which case we need this extra condition. Could you elaborate more? I think the validation logic in the extension module already validate the input well, and I cannot find a case where we need to do it.\nAre you trying to keep the logic similar to L.80?\n\nI just would like to clarify the background of your change.\n\n----\n\n(nit) We don\u0027t use backslash as possible as we can.\n\n   if (self._is_vaild_opt_value(xxx, yyy) and\n       opt[\u0027opt_value\u0027] !\u003d upd_rec[\u0027opt_value\u0027]):","commit_id":"fe86a8e36d3c64fec045c26ff52dad99bc0bcc6d"},{"author":{"_account_id":15427,"name":"David Edery","email":"david.edery@gmail.com","username":"dedery"},"change_message_id":"b82b6408174a85c17f01a409b6b45e1e0c470a2c","unresolved":false,"context_lines":[{"line_number":123,"context_line":"                                if self._is_valid_opt_value("},{"line_number":124,"context_line":"                                        opt[\u0027opt_name\u0027],"},{"line_number":125,"context_line":"                                        upd_rec[\u0027opt_value\u0027]) and \\"},{"line_number":126,"context_line":"                                   (opt[\u0027opt_value\u0027] !\u003d upd_rec[\u0027opt_value\u0027]):"},{"line_number":127,"context_line":"                                    opt.update("},{"line_number":128,"context_line":"                                        {\u0027opt_value\u0027: upd_rec[\u0027opt_value\u0027]})"},{"line_number":129,"context_line":"                            break"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a2fa921_3ed7fb24","line":126,"in_reply_to":"7a2fa921_82cd9788","updated":"2015-10-12 13:50:57.000000000","message":"Yes. I\u0027m trying to keep the logic similar to L.80.\nYou\u0027re right to claim that the validation logic in the extension module is already validating the input well. However, if that\u0027s the case then all of this DB change is irrelevant (this is a valid claim but I\u0027m not feeling comfortable to have the validations only at the API level and not in the DB level - I\u0027d be happy to hear your thoughts about this).\n\nAs for the nit - np.","commit_id":"fe86a8e36d3c64fec045c26ff52dad99bc0bcc6d"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"164c59ee8c2664dee8167ce0049c512808752ece","unresolved":false,"context_lines":[{"line_number":124,"context_line":"                                        opt[\u0027opt_name\u0027],"},{"line_number":125,"context_line":"                                        upd_rec[\u0027opt_value\u0027]) and"},{"line_number":126,"context_line":"                                        opt[\u0027opt_value\u0027] !\u003d"},{"line_number":127,"context_line":"                                        upd_rec[\u0027opt_value\u0027]):"},{"line_number":128,"context_line":"                                    opt.update("},{"line_number":129,"context_line":"                                        {\u0027opt_value\u0027: upd_rec[\u0027opt_value\u0027]})"},{"line_number":130,"context_line":"                            break"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a2ca52d_4dd8b8ef","line":127,"updated":"2015-10-15 08:32:42.000000000","message":"Do you really need to wrap lines? Particually L.126-127 and L.124-125.","commit_id":"f361f35a26c7a8f377475a03718a1fc31c748974"},{"author":{"_account_id":15427,"name":"David Edery","email":"david.edery@gmail.com","username":"dedery"},"change_message_id":"862838d59d688c7a4df11201582a733a77968f6b","unresolved":false,"context_lines":[{"line_number":124,"context_line":"                                        opt[\u0027opt_name\u0027],"},{"line_number":125,"context_line":"                                        upd_rec[\u0027opt_value\u0027]) and"},{"line_number":126,"context_line":"                                        opt[\u0027opt_value\u0027] !\u003d"},{"line_number":127,"context_line":"                                        upd_rec[\u0027opt_value\u0027]):"},{"line_number":128,"context_line":"                                    opt.update("},{"line_number":129,"context_line":"                                        {\u0027opt_value\u0027: upd_rec[\u0027opt_value\u0027]})"},{"line_number":130,"context_line":"                            break"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a2ca52d_80ce97b2","line":127,"in_reply_to":"5a2ca52d_4dd8b8ef","updated":"2015-10-15 08:53:12.000000000","message":"Yes.\nThis is what pep8 has to say about it:\n./neutron/db/extradhcpopt_db.py:124:80: E501 line too long (82 \u003e 79 characters)\n                                        opt[\u0027opt_name\u0027], upd_rec[\u0027opt_value\u0027]) and\n                                                                               ^\n./neutron/db/extradhcpopt_db.py:125:80: E501 line too long (82 \u003e 79 characters)\n                                        opt[\u0027opt_value\u0027] !\u003d upd_rec[\u0027opt_value\u0027]):\n                                                                               ^","commit_id":"f361f35a26c7a8f377475a03718a1fc31c748974"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d457245640521e66d263345974d491db4078fb45","unresolved":false,"context_lines":[{"line_number":61,"context_line":"    \"\"\""},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def _is_valid_opt_value(self, opt_name, opt_value):"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"        # If the dhcp opt is blank-able, it shouldn\u0027t be saved to the DB in"},{"line_number":66,"context_line":"        # case that the value is None"},{"line_number":67,"context_line":"        if opt_name in edo_ext.VALID_BLANK_EXTRA_DHCP_OPTS:"}],"source_content_type":"text/x-python","patch_set":15,"id":"3a29b11f_eaeb0877","line":64,"updated":"2015-10-23 20:50:08.000000000","message":"nit:  extra space","commit_id":"3837427189a54e6c052d773d629bac53d95702dc"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d457245640521e66d263345974d491db4078fb45","unresolved":false,"context_lines":[{"line_number":118,"context_line":"                                    \u0027ip_version\u0027, 4)):"},{"line_number":119,"context_line":"                            # to handle deleting of a opt from the port."},{"line_number":120,"context_line":"                            if upd_rec[\u0027opt_value\u0027] is None:"},{"line_number":121,"context_line":"                                context.session.delete(opt)"},{"line_number":122,"context_line":"                            else:"},{"line_number":123,"context_line":"                                if (self._is_valid_opt_value("},{"line_number":124,"context_line":"                                        opt[\u0027opt_name\u0027],"}],"source_content_type":"text/x-python","patch_set":15,"id":"3a29b11f_ca564cce","line":121,"updated":"2015-10-23 20:50:08.000000000","message":"Random observation not really related to this patch.  No action requested:\n\nThe indentation in this method is a bit crazy.  The if on L108 could change to this and the rest of the method indented one fewer level:\n\n  if not dopts:\n      return False\n\nThen, for example, the for loop on L115 could change to reduce indentation and improve readability IMO from this pattern:\n\n  for opt in opt_db:\n      if \u003cconditional\u003e\n          \u003cdo stuff\u003e\n          break\n  else:\n      \u003cother stuff\u003e\n\nTo something like this:  \n\n  try:\n      opt \u003d six.next(opt for opt in opt_db where \u003cconditional\u003e)\n      \u003cdo stuff\u003e\n  except StopIteration:\n      \u003cother stuff\u003e\n\nI\u0027m just making an observation.  I\u0027m not saying whether it is worth a refactor or not.  But, that is the way I would\u0027ve written it from scratch.  Don\u0027t take this as a request to refactor this, especially not as part of this patch.  That wouldn\u0027t even be appropriate.  :)","commit_id":"3837427189a54e6c052d773d629bac53d95702dc"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d457245640521e66d263345974d491db4078fb45","unresolved":false,"context_lines":[{"line_number":120,"context_line":"                            if upd_rec[\u0027opt_value\u0027] is None:"},{"line_number":121,"context_line":"                                context.session.delete(opt)"},{"line_number":122,"context_line":"                            else:"},{"line_number":123,"context_line":"                                if (self._is_valid_opt_value("},{"line_number":124,"context_line":"                                        opt[\u0027opt_name\u0027],"},{"line_number":125,"context_line":"                                        upd_rec[\u0027opt_value\u0027]) and"},{"line_number":126,"context_line":"                                        opt[\u0027opt_value\u0027] !\u003d"}],"source_content_type":"text/x-python","patch_set":15,"id":"3a29b11f_ea44681c","line":123,"updated":"2015-10-23 20:50:08.000000000","message":"nit:  I wouldn\u0027t mind a local variable to help readability here.  Consider it if you get a chance to do another revision.  Something like this.\n\n  valid \u003d self._is_valid_opt_value(opt[\u0027opt_name\u0027], upd_rec[\u0027opt_value\u0027]) \n  if valid and opt[\u0027opt_value\u0027] !\u003d upd_rec[\u0027opt_value\u0027]):","commit_id":"3837427189a54e6c052d773d629bac53d95702dc"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d457245640521e66d263345974d491db4078fb45","unresolved":false,"context_lines":[{"line_number":129,"context_line":"                                        {\u0027opt_value\u0027: upd_rec[\u0027opt_value\u0027]})"},{"line_number":130,"context_line":"                            break"},{"line_number":131,"context_line":"                    else:"},{"line_number":132,"context_line":"                        if self._is_valid_opt_value("},{"line_number":133,"context_line":"                                upd_rec[\u0027opt_name\u0027],"},{"line_number":134,"context_line":"                                upd_rec[\u0027opt_value\u0027]):"},{"line_number":135,"context_line":"                            ip_version \u003d upd_rec.get(\u0027ip_version\u0027, 4)"}],"source_content_type":"text/x-python","patch_set":15,"id":"3a29b11f_8a4a9426","line":132,"updated":"2015-10-23 20:50:08.000000000","message":"ditto","commit_id":"3837427189a54e6c052d773d629bac53d95702dc"}],"neutron/extensions/extra_dhcp_opt.py":[{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"d72ebcdce9c043f4d48a2420a1711a3af61ba829","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"# Valid blank extra dhcp opts"},{"line_number":31,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, )"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def _validate_list_of_dict_or_none(data, key_specs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a1a9d01_3e519a36","line":31,"updated":"2015-09-29 23:09:38.000000000","message":"Why router only and not also classless-static-route?","commit_id":"cd7694c676732210b0c15290461bdf16b07a34e6"},{"author":{"_account_id":7715,"name":"Hirofumi Ichihara","email":"ichihara.hirofumi@gmail.com","username":"Hirofumi-Ichihara"},"change_message_id":"378bc880eb2bbd5683878622f6b9482f0a45f17d","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"# Valid blank extra dhcp opts"},{"line_number":31,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, \u0027classless-static-route\u0027, )"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def _validate_list_of_dict_or_none(data, key_specs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a1a9d01_37fbc248","line":31,"updated":"2015-10-01 07:59:53.000000000","message":"The last comma is unnecessary.","commit_id":"63e7d1ebb0064b106d4ac456341db729264e3606"},{"author":{"_account_id":14611,"name":"dasm","display_name":"dasm","email":"dsmigiel@redhat.com","username":"dasm"},"change_message_id":"95c78ece8c6e2838c098364f2880af9cf6476304","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"# Valid blank extra dhcp opts"},{"line_number":31,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, \u0027classless-static-route\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def _validate_list_of_dict_or_none(data, key_specs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a2fa921_f693f5fe","line":31,"updated":"2015-10-12 09:44:06.000000000","message":"I see some constants below this line, and IDK how Neutron community is about this, but it would be nice to have all constants at top of the module. Could you move it?","commit_id":"0640e52d9303413419b545bdc03bfff102c9d060"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"698ed516c9eadd58ffd0dd27627751d70653cdd0","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"# Valid blank extra dhcp opts"},{"line_number":31,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, \u0027classless-static-route\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def _validate_list_of_dict_or_none(data, key_specs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a2fa921_c79de10b","line":31,"in_reply_to":"7a2fa921_ace3ec04","updated":"2015-10-12 10:56:13.000000000","message":"I think having all definitions in the top module can lead to a mess. IMO it is good to have this definition here because we have definitions specific to features (extensions in most cases now) in extension modules. Thought?","commit_id":"0640e52d9303413419b545bdc03bfff102c9d060"},{"author":{"_account_id":15427,"name":"David Edery","email":"david.edery@gmail.com","username":"dedery"},"change_message_id":"de84c07673270716518b42d738d2da00a54eca90","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"# Valid blank extra dhcp opts"},{"line_number":31,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, \u0027classless-static-route\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def _validate_list_of_dict_or_none(data, key_specs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a2fa921_825d974a","line":31,"in_reply_to":"7a2fa921_c79de10b","updated":"2015-10-12 11:18:37.000000000","message":"+1\nSince we share the same view (about having the definition next to the usage) I\u0027ll take it back to where it was.","commit_id":"0640e52d9303413419b545bdc03bfff102c9d060"},{"author":{"_account_id":15427,"name":"David Edery","email":"david.edery@gmail.com","username":"dedery"},"change_message_id":"1bfb078c3f1750ee11d6132f655aa0b7abdf75c8","unresolved":false,"context_lines":[{"line_number":28,"context_line":""},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"# Valid blank extra dhcp opts"},{"line_number":31,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, \u0027classless-static-route\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def _validate_list_of_dict_or_none(data, key_specs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a2fa921_ace3ec04","line":31,"in_reply_to":"7a2fa921_f693f5fe","updated":"2015-10-12 10:18:32.000000000","message":"In previous reviews by cores no one commented about the place of the constant. It seemed logical to put it next to its usage. \nI don\u0027t mind moving it to the top.","commit_id":"0640e52d9303413419b545bdc03bfff102c9d060"},{"author":{"_account_id":14611,"name":"dasm","display_name":"dasm","email":"dsmigiel@redhat.com","username":"dasm"},"change_message_id":"e2eafe71834fea751bc841fa51f5f844ad15ae98","unresolved":false,"context_lines":[{"line_number":18,"context_line":"from neutron.common import exceptions"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"# Valid blank extra dhcp opts"},{"line_number":21,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, \u0027classless-static-route\u0027)"},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"# ExtraDHcpOpts Exceptions"},{"line_number":24,"context_line":"class ExtraDhcpOptNotFound(exceptions.NotFound):"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a2fa921_38f2b370","line":21,"updated":"2015-10-12 13:22:32.000000000","message":"David, Akihiro, if you think the better thing for this constant is to have everything in one place, I\u0027m ok with this.","commit_id":"fe86a8e36d3c64fec045c26ff52dad99bc0bcc6d"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"164c59ee8c2664dee8167ce0049c512808752ece","unresolved":false,"context_lines":[{"line_number":31,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, \u0027classless-static-route\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def _validate_list_of_dict_or_none(data, key_specs\u003dNone):"},{"line_number":35,"context_line":"    if data is not None:"},{"line_number":36,"context_line":"        if not isinstance(data, list):"},{"line_number":37,"context_line":"            raise ExtraDhcpOptBadData(data\u003ddata)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a2ca52d_ca759398","line":34,"updated":"2015-10-15 08:32:42.000000000","message":"Could you rename the method name, e.g., _validate_extra_dhcp_opt?\nThis method now contains a special logic, so the method name no longer describes what it does.","commit_id":"f361f35a26c7a8f377475a03718a1fc31c748974"},{"author":{"_account_id":15427,"name":"David Edery","email":"david.edery@gmail.com","username":"dedery"},"change_message_id":"862838d59d688c7a4df11201582a733a77968f6b","unresolved":false,"context_lines":[{"line_number":31,"context_line":"VALID_BLANK_EXTRA_DHCP_OPTS \u003d (\u0027router\u0027, \u0027classless-static-route\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"def _validate_list_of_dict_or_none(data, key_specs\u003dNone):"},{"line_number":35,"context_line":"    if data is not None:"},{"line_number":36,"context_line":"        if not isinstance(data, list):"},{"line_number":37,"context_line":"            raise ExtraDhcpOptBadData(data\u003ddata)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a2ca52d_e0bf4bf9","line":34,"in_reply_to":"5a2ca52d_ca759398","updated":"2015-10-15 08:53:12.000000000","message":"np","commit_id":"f361f35a26c7a8f377475a03718a1fc31c748974"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"164c59ee8c2664dee8167ce0049c512808752ece","unresolved":false,"context_lines":[{"line_number":40,"context_line":"                msg \u003d attr._validate_string_or_none(d[\u0027opt_value\u0027],"},{"line_number":41,"context_line":"                                                    DHCP_OPT_VALUE_MAX_LEN)"},{"line_number":42,"context_line":"            else:"},{"line_number":43,"context_line":"                msg \u003d attr._validate_dict(d, key_specs)"},{"line_number":44,"context_line":"            if msg:"},{"line_number":45,"context_line":"                raise ExtraDhcpOptBadData(data\u003dmsg)"},{"line_number":46,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"5a2ca52d_4a6ae3b4","line":43,"updated":"2015-10-15 08:32:42.000000000","message":"In addition to the above comment, I think it is time to move \"key_specs\" at L\u003e68-76 inside this method because we have an extra logic here and it is better to define all things in one place.\n\nWhile reviewing this patch, I felt confused several times.","commit_id":"f361f35a26c7a8f377475a03718a1fc31c748974"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d457245640521e66d263345974d491db4078fb45","unresolved":false,"context_lines":[{"line_number":69,"context_line":""},{"line_number":70,"context_line":"EXTENDED_ATTRIBUTES_2_0 \u003d {"},{"line_number":71,"context_line":"    \u0027ports\u0027: {"},{"line_number":72,"context_line":"        EXTRADHCPOPTS: {"},{"line_number":73,"context_line":"            \u0027allow_post\u0027: True,"},{"line_number":74,"context_line":"            \u0027allow_put\u0027: True,"},{"line_number":75,"context_line":"            \u0027is_visible\u0027: True,"}],"source_content_type":"text/x-python","patch_set":15,"id":"3a29b11f_aafe7036","line":72,"updated":"2015-10-23 20:50:08.000000000","message":"I would strongly prefer you make these changes without changes all the whitespace around it more than necessary.","commit_id":"3837427189a54e6c052d773d629bac53d95702dc"}],"neutron/tests/unit/extensions/test_extra_dhcp_opt.py":[{"author":{"_account_id":7715,"name":"Hirofumi Ichihara","email":"ichihara.hirofumi@gmail.com","username":"Hirofumi-Ichihara"},"change_message_id":"378bc880eb2bbd5683878622f6b9482f0a45f17d","unresolved":false,"context_lines":[{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    def test_create_port_with_empty_router_extradhcpopts(self):"},{"line_number":110,"context_line":"        opt_list \u003d [{\u0027opt_name\u0027: \u0027router\u0027,"},{"line_number":111,"context_line":"                     \u0027opt_value\u0027: \u0027\u0027},"},{"line_number":112,"context_line":"                    {\u0027opt_name\u0027: \u0027server-ip-address\u0027,"},{"line_number":113,"context_line":"                     \u0027opt_value\u0027: \u0027123.123.123.456\u0027},"},{"line_number":114,"context_line":"                    {\u0027opt_name\u0027: \u0027tftp-server\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a1a9d01_d27db46d","line":111,"updated":"2015-10-01 07:59:53.000000000","message":"I\u0027m not sure which does this option except empty string or None. In neutron/extensions/extra_dhcp_opt.py, it seems this patch allows opt_value to be None. What do you expect?","commit_id":"63e7d1ebb0064b106d4ac456341db729264e3606"}]}