)]}'
{"lib/ip.py":[{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":85,"context_line":"    more than one `,` is found, the range will be between the first entry and"},{"line_number":86,"context_line":"    the last, ignoring any specified in between."},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    Deduplication is not handled by this function; only reading configuration"},{"line_number":89,"context_line":"    data."},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    :param user_defined_config: ``dict`` Uses the \u0027used_ips\u0027 key as a source"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_95c34c38","line":88,"updated":"2016-12-07 20:58:23.000000000","message":"Why is the de-duplication no longer handled here? Does that allow additional useful information to be communicated?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"300d5a2f83d3ee8b83f9cdc6b7a3eda122360f9e","unresolved":false,"context_lines":[{"line_number":85,"context_line":"    more than one `,` is found, the range will be between the first entry and"},{"line_number":86,"context_line":"    the last, ignoring any specified in between."},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    Deduplication is not handled by this function; only reading configuration"},{"line_number":89,"context_line":"    data."},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    :param user_defined_config: ``dict`` Uses the \u0027used_ips\u0027 key as a source"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_c58ec8cf","line":88,"in_reply_to":"3a71b18c_029ef6b7","updated":"2016-12-08 16:19:37.000000000","message":"Given that we\u0027re talking about using a set instead of a list I\u0027m not sure I see it as a waste. Unless the presence of duplicates is intended to communicate something it strikes me as a less \u0027clean\u0027 solution and given that there is no context provided as to why there might be duplicates it doesn\u0027t seem beneficial. I think if someone was going by the function name, they wouldn\u0027t expect to have to do further clean up on what is returned.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"f176fe97395fb8906c1dbc5803e079ad5f296dc5","unresolved":false,"context_lines":[{"line_number":85,"context_line":"    more than one `,` is found, the range will be between the first entry and"},{"line_number":86,"context_line":"    the last, ignoring any specified in between."},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    Deduplication is not handled by this function; only reading configuration"},{"line_number":89,"context_line":"    data."},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    :param user_defined_config: ``dict`` Uses the \u0027used_ips\u0027 key as a source"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_029ef6b7","line":88,"in_reply_to":"3a71b18c_46d029ca","updated":"2016-12-08 15:41:00.000000000","message":"What Steve said is correct - deduplication is done by the set_used_ips functions.\n\nDoing deduplication here is kind of a waste anyway, because the USED_IPS and individual host IPs may overlap.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"31ed1cd4ec056ab3cf04b518dc2ed3e8684dd030","unresolved":false,"context_lines":[{"line_number":85,"context_line":"    more than one `,` is found, the range will be between the first entry and"},{"line_number":86,"context_line":"    the last, ignoring any specified in between."},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    Deduplication is not handled by this function; only reading configuration"},{"line_number":89,"context_line":"    data."},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    :param user_defined_config: ``dict`` Uses the \u0027used_ips\u0027 key as a source"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_46d029ca","line":88,"in_reply_to":"3a71b18c_95c34c38","updated":"2016-12-07 21:29:47.000000000","message":"As a separation of concerns issue, where de-duplication should occur is something I was treating as a future concern. Maybe that wasn\u0027t the best perspective given the notes below.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":93,"context_line":"    \"\"\""},{"line_number":94,"context_line":"    ips_to_add \u003d []"},{"line_number":95,"context_line":"    used_ips \u003d user_defined_config.get(\u0027used_ips\u0027)"},{"line_number":96,"context_line":"    if isinstance(used_ips, list):"},{"line_number":97,"context_line":"        for ip in used_ips:"},{"line_number":98,"context_line":"            split_ip \u003d ip.split(\u0027,\u0027)"},{"line_number":99,"context_line":"            if len(split_ip) \u003e\u003d 2:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_7a85bfab","line":96,"updated":"2016-12-07 20:58:23.000000000","message":"Maybe this should fail if it\u0027s not a list, this is essentially failing silently.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"300d5a2f83d3ee8b83f9cdc6b7a3eda122360f9e","unresolved":false,"context_lines":[{"line_number":93,"context_line":"    \"\"\""},{"line_number":94,"context_line":"    ips_to_add \u003d []"},{"line_number":95,"context_line":"    used_ips \u003d user_defined_config.get(\u0027used_ips\u0027)"},{"line_number":96,"context_line":"    if isinstance(used_ips, list):"},{"line_number":97,"context_line":"        for ip in used_ips:"},{"line_number":98,"context_line":"            split_ip \u003d ip.split(\u0027,\u0027)"},{"line_number":99,"context_line":"            if len(split_ip) \u003e\u003d 2:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_109e3cb8","line":96,"in_reply_to":"3a71b18c_22a5b203","updated":"2016-12-08 16:19:37.000000000","message":"It\u0027s not about USED_IPS but used_ips. This test implies that if used_ips isn\u0027t a list then no used IP addresses were defined. What I\u0027m suggesting is that if there are values, other that an empty list, that can be used to show that there are no used IP addresses, they should be defined explicitly. The idea being to account for when used_ips is mistakenly configured to the wrong thing and it just gets ignored. So for example:\n\nif isinstance(used_ips, list):\n    pass\nelif used_ips is not None:\n    ValueError(\u0027used_ips must be a list or IP addresses or null\u0027)","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"34ef6975dc52d81a15bd6ea905b8b5e86bd8ae5a","unresolved":false,"context_lines":[{"line_number":93,"context_line":"    \"\"\""},{"line_number":94,"context_line":"    ips_to_add \u003d []"},{"line_number":95,"context_line":"    used_ips \u003d user_defined_config.get(\u0027used_ips\u0027)"},{"line_number":96,"context_line":"    if isinstance(used_ips, list):"},{"line_number":97,"context_line":"        for ip in used_ips:"},{"line_number":98,"context_line":"            split_ip \u003d ip.split(\u0027,\u0027)"},{"line_number":99,"context_line":"            if len(split_ip) \u003e\u003d 2:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_867cd321","line":96,"in_reply_to":"3a71b18c_22a5b203","updated":"2016-12-08 17:51:27.000000000","message":"Just as an idea: An empty list is a list, and smart placeholder values (such as None) can be interpolated as an empty list explicitly. A failure mesage on other stuff should be enough to nudge folks in the right direction.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"31ed1cd4ec056ab3cf04b518dc2ed3e8684dd030","unresolved":false,"context_lines":[{"line_number":93,"context_line":"    \"\"\""},{"line_number":94,"context_line":"    ips_to_add \u003d []"},{"line_number":95,"context_line":"    used_ips \u003d user_defined_config.get(\u0027used_ips\u0027)"},{"line_number":96,"context_line":"    if isinstance(used_ips, list):"},{"line_number":97,"context_line":"        for ip in used_ips:"},{"line_number":98,"context_line":"            split_ip \u003d ip.split(\u0027,\u0027)"},{"line_number":99,"context_line":"            if len(split_ip) \u003e\u003d 2:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_e6f8554c","line":96,"in_reply_to":"3a71b18c_7a85bfab","updated":"2016-12-07 21:29:47.000000000","message":"+1 \n\nused_ips is a required element and that is enforced elsewhere, but a useful failure message if it isn\u0027t presented as a list would be helpful here.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"f176fe97395fb8906c1dbc5803e079ad5f296dc5","unresolved":false,"context_lines":[{"line_number":93,"context_line":"    \"\"\""},{"line_number":94,"context_line":"    ips_to_add \u003d []"},{"line_number":95,"context_line":"    used_ips \u003d user_defined_config.get(\u0027used_ips\u0027)"},{"line_number":96,"context_line":"    if isinstance(used_ips, list):"},{"line_number":97,"context_line":"        for ip in used_ips:"},{"line_number":98,"context_line":"            split_ip \u003d ip.split(\u0027,\u0027)"},{"line_number":99,"context_line":"            if len(split_ip) \u003e\u003d 2:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_22a5b203","line":96,"in_reply_to":"3a71b18c_e6f8554c","updated":"2016-12-08 15:41:00.000000000","message":"USED_IPS can be empty from the stance of the configuration; why should it fail in that case?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":98,"context_line":"            split_ip \u003d ip.split(\u0027,\u0027)"},{"line_number":99,"context_line":"            if len(split_ip) \u003e\u003d 2:"},{"line_number":100,"context_line":"                ip_range \u003d list("},{"line_number":101,"context_line":"                    netaddr.iter_iprange("},{"line_number":102,"context_line":"                        split_ip[0],"},{"line_number":103,"context_line":"                        split_ip[-1]"},{"line_number":104,"context_line":"                    )"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_d0614fd5","line":101,"updated":"2016-12-07 20:58:23.000000000","message":"Where do IP addresses get validated? I guess it happens here as a side effect of providing a range but I can\u0027t see it done explicitly. If that\u0027s the case, does that mean individual IP addresses (the else block) don\u0027t get validated? Is the inventory code designed to be as dumb as possible with the expectation that the playbooks are the only thing that can know where or not a value is correct?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"f176fe97395fb8906c1dbc5803e079ad5f296dc5","unresolved":false,"context_lines":[{"line_number":98,"context_line":"            split_ip \u003d ip.split(\u0027,\u0027)"},{"line_number":99,"context_line":"            if len(split_ip) \u003e\u003d 2:"},{"line_number":100,"context_line":"                ip_range \u003d list("},{"line_number":101,"context_line":"                    netaddr.iter_iprange("},{"line_number":102,"context_line":"                        split_ip[0],"},{"line_number":103,"context_line":"                        split_ip[-1]"},{"line_number":104,"context_line":"                    )"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_2220529b","line":101,"in_reply_to":"3a71b18c_d0614fd5","updated":"2016-12-08 15:41:00.000000000","message":"IPs aren\u0027t validated, no. I think doing so is outside the scope of this particular patch.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":10881,"name":"Neill Cox","email":"neill@ingenious.com.au","username":"neillc"},"change_message_id":"ae6ab4a94b2516b51a2cd1ff869a935c57a5131e","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                        split_ip[-1]"},{"line_number":104,"context_line":"                    )"},{"line_number":105,"context_line":"                )"},{"line_number":106,"context_line":"                ips_to_add.extend([str(i) for i in ip_range])"},{"line_number":107,"context_line":"            else:"},{"line_number":108,"context_line":"                logger.debug(\"IP %s set as used\", split_ip[0])"},{"line_number":109,"context_line":"                ips_to_add.append(split_ip[0])"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_dfeccef5","line":106,"updated":"2016-11-30 21:25:38.000000000","message":"is it worth logging these IP for consistency?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                        split_ip[-1]"},{"line_number":104,"context_line":"                    )"},{"line_number":105,"context_line":"                )"},{"line_number":106,"context_line":"                ips_to_add.extend([str(i) for i in ip_range])"},{"line_number":107,"context_line":"            else:"},{"line_number":108,"context_line":"                logger.debug(\"IP %s set as used\", split_ip[0])"},{"line_number":109,"context_line":"                ips_to_add.append(split_ip[0])"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_b54a08ab","line":106,"in_reply_to":"3a71b18c_7da564b3","updated":"2016-12-07 20:58:23.000000000","message":"I agree is would make sense to log the ranges, given this only uses first and last maybe it would be useful to log anything ignored as well.\n\nnit: extend can take a generator expression\n\nips_to_add.extend(str(i) for i in ip_range)","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"e562648c8c65010f4c838448af0aa1592dc66e22","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                        split_ip[-1]"},{"line_number":104,"context_line":"                    )"},{"line_number":105,"context_line":"                )"},{"line_number":106,"context_line":"                ips_to_add.extend([str(i) for i in ip_range])"},{"line_number":107,"context_line":"            else:"},{"line_number":108,"context_line":"                logger.debug(\"IP %s set as used\", split_ip[0])"},{"line_number":109,"context_line":"                ips_to_add.append(split_ip[0])"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_61ec2f90","line":106,"in_reply_to":"3a71b18c_a10f272d","updated":"2016-12-07 21:43:57.000000000","message":"I\u0027m not sure what you\u0027re saying but it\u0027s a minor optimisation so I\u0027m not particularly worried if it\u0027s not changed.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"31ed1cd4ec056ab3cf04b518dc2ed3e8684dd030","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                        split_ip[-1]"},{"line_number":104,"context_line":"                    )"},{"line_number":105,"context_line":"                )"},{"line_number":106,"context_line":"                ips_to_add.extend([str(i) for i in ip_range])"},{"line_number":107,"context_line":"            else:"},{"line_number":108,"context_line":"                logger.debug(\"IP %s set as used\", split_ip[0])"},{"line_number":109,"context_line":"                ips_to_add.append(split_ip[0])"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_a10f272d","line":106,"in_reply_to":"3a71b18c_b54a08ab","updated":"2016-12-07 21:29:47.000000000","message":"regarding the nit, that is true but I feel like the given syntax may be less subtle because it communicates type information in the explicit use of list comprehension and thus may be better in this context. I have no strong opinion though.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"30f38193d544ea6bf768a9ad10b9bdf2a612b9c1","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                        split_ip[-1]"},{"line_number":104,"context_line":"                    )"},{"line_number":105,"context_line":"                )"},{"line_number":106,"context_line":"                ips_to_add.extend([str(i) for i in ip_range])"},{"line_number":107,"context_line":"            else:"},{"line_number":108,"context_line":"                logger.debug(\"IP %s set as used\", split_ip[0])"},{"line_number":109,"context_line":"                ips_to_add.append(split_ip[0])"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_7da564b3","line":106,"in_reply_to":"5a74a57a_b56d739b","updated":"2016-12-05 17:27:10.000000000","message":"I would say adding a debug log line for the begin / end of the band would be valuable in this case. No reason to enumerate every individual one, right?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"69e01842c63189c9fdc1f947c9155ef95fcbe680","unresolved":false,"context_lines":[{"line_number":103,"context_line":"                        split_ip[-1]"},{"line_number":104,"context_line":"                    )"},{"line_number":105,"context_line":"                )"},{"line_number":106,"context_line":"                ips_to_add.extend([str(i) for i in ip_range])"},{"line_number":107,"context_line":"            else:"},{"line_number":108,"context_line":"                logger.debug(\"IP %s set as used\", split_ip[0])"},{"line_number":109,"context_line":"                ips_to_add.append(split_ip[0])"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_b56d739b","line":106,"in_reply_to":"5a74a57a_dfeccef5","updated":"2016-11-30 22:06:43.000000000","message":"These specifically/specially, or perhaps all that are added in the set_used_ips function?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":111,"context_line":"    return ips_to_add"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"def read_inventory_ips(inventory):"},{"line_number":115,"context_line":"    \"\"\"Parse IP addresses already assigned in inventory."},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"    :param inventory: ``dict`` Used to query the \u0027address\u0027 values of"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_6310b7a9","line":114,"updated":"2016-12-07 20:58:23.000000000","message":"Why are the IP addresses association with bridges maintained in the object returned? That information doesn\u0027t appear to be used. read_used_ips simply returns a list of used IP addresses, why doesn\u0027t this function do the same?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"f176fe97395fb8906c1dbc5803e079ad5f296dc5","unresolved":false,"context_lines":[{"line_number":111,"context_line":"    return ips_to_add"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"def read_inventory_ips(inventory):"},{"line_number":115,"context_line":"    \"\"\"Parse IP addresses already assigned in inventory."},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"    :param inventory: ``dict`` Used to query the \u0027address\u0027 values of"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_02027609","line":114,"in_reply_to":"3a71b18c_6310b7a9","updated":"2016-12-08 15:41:00.000000000","message":"This is being done to get ready for feeding them into the IPManager class.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":121,"context_line":"    \"\"\""},{"line_number":122,"context_line":"    # The `used_ips` list is for those addresses assigned without a bridge name"},{"line_number":123,"context_line":"    # past configurations did not mandate a bridge name was specified, so use"},{"line_number":124,"context_line":"    # this to make sure we have backwards compat"},{"line_number":125,"context_line":"    inv_ips \u003d {\u0027used_ips\u0027: []}"},{"line_number":126,"context_line":"    # Find all used IP addresses and ensure that they are not used again"},{"line_number":127,"context_line":"    for host_entry in inventory[\u0027_meta\u0027][\u0027hostvars\u0027].values():"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_8395d389","line":124,"updated":"2016-12-07 20:58:23.000000000","message":"Is this format going to be deprecated?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"f176fe97395fb8906c1dbc5803e079ad5f296dc5","unresolved":false,"context_lines":[{"line_number":121,"context_line":"    \"\"\""},{"line_number":122,"context_line":"    # The `used_ips` list is for those addresses assigned without a bridge name"},{"line_number":123,"context_line":"    # past configurations did not mandate a bridge name was specified, so use"},{"line_number":124,"context_line":"    # this to make sure we have backwards compat"},{"line_number":125,"context_line":"    inv_ips \u003d {\u0027used_ips\u0027: []}"},{"line_number":126,"context_line":"    # Find all used IP addresses and ensure that they are not used again"},{"line_number":127,"context_line":"    for host_entry in inventory[\u0027_meta\u0027][\u0027hostvars\u0027].values():"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_9f06df1e","line":124,"in_reply_to":"3a71b18c_8395d389","updated":"2016-12-08 15:41:00.000000000","message":"Which format are you referring to? The absent bridge name?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"300d5a2f83d3ee8b83f9cdc6b7a3eda122360f9e","unresolved":false,"context_lines":[{"line_number":121,"context_line":"    \"\"\""},{"line_number":122,"context_line":"    # The `used_ips` list is for those addresses assigned without a bridge name"},{"line_number":123,"context_line":"    # past configurations did not mandate a bridge name was specified, so use"},{"line_number":124,"context_line":"    # this to make sure we have backwards compat"},{"line_number":125,"context_line":"    inv_ips \u003d {\u0027used_ips\u0027: []}"},{"line_number":126,"context_line":"    # Find all used IP addresses and ensure that they are not used again"},{"line_number":127,"context_line":"    for host_entry in inventory[\u0027_meta\u0027][\u0027hostvars\u0027].values():"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_90e24c00","line":124,"in_reply_to":"3a71b18c_9f06df1e","updated":"2016-12-08 16:19:37.000000000","message":"Sorry, I wasn\u0027t very clear. I\u0027m referring to the backwards compatibility. Is support for not having a bridge name going to be removed? If so, when? Or are both ways considered equal?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":122,"context_line":"    # The `used_ips` list is for those addresses assigned without a bridge name"},{"line_number":123,"context_line":"    # past configurations did not mandate a bridge name was specified, so use"},{"line_number":124,"context_line":"    # this to make sure we have backwards compat"},{"line_number":125,"context_line":"    inv_ips \u003d {\u0027used_ips\u0027: []}"},{"line_number":126,"context_line":"    # Find all used IP addresses and ensure that they are not used again"},{"line_number":127,"context_line":"    for host_entry in inventory[\u0027_meta\u0027][\u0027hostvars\u0027].values():"},{"line_number":128,"context_line":"        networks \u003d host_entry.get(\u0027container_networks\u0027, dict())"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_23eddf25","line":125,"updated":"2016-12-07 20:58:23.000000000","message":"While it\u0027s unlikely someone would call a bridge used_ips - maybe it would be better not to allow the possibility for conflict.\n\nLooking at how this dict is used by set_used_ips it doesn\u0027t seem to matter but then I\u0027m not sure why the IP addresses aren\u0027t just added to a single list instead of kept separated by bridge.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"f176fe97395fb8906c1dbc5803e079ad5f296dc5","unresolved":false,"context_lines":[{"line_number":122,"context_line":"    # The `used_ips` list is for those addresses assigned without a bridge name"},{"line_number":123,"context_line":"    # past configurations did not mandate a bridge name was specified, so use"},{"line_number":124,"context_line":"    # this to make sure we have backwards compat"},{"line_number":125,"context_line":"    inv_ips \u003d {\u0027used_ips\u0027: []}"},{"line_number":126,"context_line":"    # Find all used IP addresses and ensure that they are not used again"},{"line_number":127,"context_line":"    for host_entry in inventory[\u0027_meta\u0027][\u0027hostvars\u0027].values():"},{"line_number":128,"context_line":"        networks \u003d host_entry.get(\u0027container_networks\u0027, dict())"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_c28b1e67","line":125,"in_reply_to":"3a71b18c_23eddf25","updated":"2016-12-08 15:41:00.000000000","message":"Again, this is getting them ready for the IPManager class.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"30f38193d544ea6bf768a9ad10b9bdf2a612b9c1","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            bridge \u003d network_entry.get(\u0027bridge\u0027)"},{"line_number":131,"context_line":"            if bridge is None:"},{"line_number":132,"context_line":"                bridge \u003d \u0027used_ips\u0027"},{"line_number":133,"context_line":"            if bridge and bridge not in inv_ips:"},{"line_number":134,"context_line":"                inv_ips[bridge] \u003d []"},{"line_number":135,"context_line":"            address \u003d network_entry.get(\u0027address\u0027)"},{"line_number":136,"context_line":"            if address and address not in inv_ips[bridge]:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_51da1ae9","line":133,"range":{"start_line":133,"start_character":12,"end_line":133,"end_character":47},"updated":"2016-12-05 17:27:10.000000000","message":"would an else if construct be clearer here? Invert the order? I\u0027m not sure but it may","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":130,"context_line":"            bridge \u003d network_entry.get(\u0027bridge\u0027)"},{"line_number":131,"context_line":"            if bridge is None:"},{"line_number":132,"context_line":"                bridge \u003d \u0027used_ips\u0027"},{"line_number":133,"context_line":"            if bridge and bridge not in inv_ips:"},{"line_number":134,"context_line":"                inv_ips[bridge] \u003d []"},{"line_number":135,"context_line":"            address \u003d network_entry.get(\u0027address\u0027)"},{"line_number":136,"context_line":"            if address and address not in inv_ips[bridge]:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_50afff7c","line":133,"range":{"start_line":133,"start_character":12,"end_line":133,"end_character":47},"in_reply_to":"3a71b18c_51da1ae9","updated":"2016-12-07 20:58:23.000000000","message":"Alternatively use defaultdict and you can do away with this if block, e.g.\n\ninv_ips \u003d defaultdict(list)","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"30f38193d544ea6bf768a9ad10b9bdf2a612b9c1","unresolved":false,"context_lines":[{"line_number":132,"context_line":"                bridge \u003d \u0027used_ips\u0027"},{"line_number":133,"context_line":"            if bridge and bridge not in inv_ips:"},{"line_number":134,"context_line":"                inv_ips[bridge] \u003d []"},{"line_number":135,"context_line":"            address \u003d network_entry.get(\u0027address\u0027)"},{"line_number":136,"context_line":"            if address and address not in inv_ips[bridge]:"},{"line_number":137,"context_line":"                logger.debug(\"IP %s set as used\", address)"},{"line_number":138,"context_line":"                inv_ips[bridge].append(address)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_fc54ddfe","line":135,"updated":"2016-12-05 17:27:10.000000000","message":"maybe add whitespace between 134 and 135?\n\nanother idea would be an `_ensure_bridge_is_known` func (name tbd), consisting of lines 130:134","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":133,"context_line":"            if bridge and bridge not in inv_ips:"},{"line_number":134,"context_line":"                inv_ips[bridge] \u003d []"},{"line_number":135,"context_line":"            address \u003d network_entry.get(\u0027address\u0027)"},{"line_number":136,"context_line":"            if address and address not in inv_ips[bridge]:"},{"line_number":137,"context_line":"                logger.debug(\"IP %s set as used\", address)"},{"line_number":138,"context_line":"                inv_ips[bridge].append(address)"},{"line_number":139,"context_line":"    return inv_ips"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_10bbc776","line":136,"updated":"2016-12-07 20:58:23.000000000","message":"If inv_ips[bridge] was a set you could simplify this. It would also mean that multiple additions of the same IP address get logged but that may be useful. You are effectively de-duplicating here but that is explicitly not done in read_used_ips - why?\n\nHere there is no test for bool(bridge) is True whereas line 133 does. Given that None has already been accounted for this leaves things like \u0027\u0027 and 0 which wouldn\u0027t be added to inv_ips on line 134 and so would cause a KeyError here. I\u0027d say either define a valid bridge and check for it but if this is trying to be as loose as possible then 133 and 136 should at least be consistent.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":152,"context_line":""},{"line_number":153,"context_line":"    inv_ips \u003d read_inventory_ips(inventory)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    # Merge IPs that didn\u0027t have a bridge name assigned into the plain used_ips"},{"line_number":156,"context_line":"    # list."},{"line_number":157,"context_line":"    unassoc_ips \u003d inv_ips.pop(\u0027used_ips\u0027)"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    conf_ips.extend(unassoc_ips)"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    # TODO(nrb): replace this with creating an IPManager instance like:"},{"line_number":162,"context_line":"    # return IPManager(queues\u003dinv_ips, used_ips\u003dconf_ips)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_03d2a334","line":159,"range":{"start_line":155,"start_character":0,"end_line":159,"end_character":32},"updated":"2016-12-07 20:58:23.000000000","message":"Is there any benefit to doing this? It seems that used_ips get treated the same as a bridge, why not just leave it to the for-loop?","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"f176fe97395fb8906c1dbc5803e079ad5f296dc5","unresolved":false,"context_lines":[{"line_number":152,"context_line":""},{"line_number":153,"context_line":"    inv_ips \u003d read_inventory_ips(inventory)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"    # Merge IPs that didn\u0027t have a bridge name assigned into the plain used_ips"},{"line_number":156,"context_line":"    # list."},{"line_number":157,"context_line":"    unassoc_ips \u003d inv_ips.pop(\u0027used_ips\u0027)"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    conf_ips.extend(unassoc_ips)"},{"line_number":160,"context_line":""},{"line_number":161,"context_line":"    # TODO(nrb): replace this with creating an IPManager instance like:"},{"line_number":162,"context_line":"    # return IPManager(queues\u003dinv_ips, used_ips\u003dconf_ips)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_e2d2da33","line":159,"range":{"start_line":155,"start_character":0,"end_line":159,"end_character":32},"in_reply_to":"3a71b18c_03d2a334","updated":"2016-12-08 15:41:00.000000000","message":"This is somewhat left over from my initial pass, which added things to the IPManager class. I could roll this into the loop, though I would be pulling it right back out for the IPManager integration.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":162,"context_line":"    # return IPManager(queues\u003dinv_ips, used_ips\u003dconf_ips)"},{"line_number":163,"context_line":"    USED_IPS.update(conf_ips)"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    for bridge, ips in inv_ips.items():"},{"line_number":166,"context_line":"        USED_IPS.update(ips)"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_e3d767eb","line":165,"updated":"2016-12-07 20:58:23.000000000","message":"nit: given that bridge is unused, this could be changed to:\n\nfor ips in inv_ips.values():","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"94a41998229f34e7ec19b3e53f62f4ac253378b6","unresolved":false,"context_lines":[{"line_number":163,"context_line":"    USED_IPS.update(conf_ips)"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    for bridge, ips in inv_ips.items():"},{"line_number":166,"context_line":"        USED_IPS.update(ips)"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"class NoSuchQueue(Exception):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_353fd8d7","line":166,"updated":"2016-12-07 20:58:23.000000000","message":"There doesn\u0027t appear to be any need for this to be a global variable, I think it would be better for set_used_ips to return an appropriate object.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"31ed1cd4ec056ab3cf04b518dc2ed3e8684dd030","unresolved":false,"context_lines":[{"line_number":163,"context_line":"    USED_IPS.update(conf_ips)"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    for bridge, ips in inv_ips.items():"},{"line_number":166,"context_line":"        USED_IPS.update(ips)"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"class NoSuchQueue(Exception):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_a1e3a704","line":166,"in_reply_to":"3a71b18c_353fd8d7","updated":"2016-12-07 21:29:47.000000000","message":"That will happen but we are explicitly trying to keep patch sets small and impacts minimal so I understand why this was deferred.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":12892,"name":"Nolan Brubaker","email":"nolan.brubaker@rackspace.com","username":"nrb"},"change_message_id":"f176fe97395fb8906c1dbc5803e079ad5f296dc5","unresolved":false,"context_lines":[{"line_number":163,"context_line":"    USED_IPS.update(conf_ips)"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    for bridge, ips in inv_ips.items():"},{"line_number":166,"context_line":"        USED_IPS.update(ips)"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"class NoSuchQueue(Exception):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_8277663a","line":166,"in_reply_to":"3a71b18c_816ceb37","updated":"2016-12-08 15:41:00.000000000","message":"The goal is to return an appropriate object, yes, but I found that just returning an IPManager instance would require editing a lot more code, which has been distasteful to reviewers in the past.\n\nUSED_IPS is set in order to be compatible with how we do things now, but the function exists to get ready for switching.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"e562648c8c65010f4c838448af0aa1592dc66e22","unresolved":false,"context_lines":[{"line_number":163,"context_line":"    USED_IPS.update(conf_ips)"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    for bridge, ips in inv_ips.items():"},{"line_number":166,"context_line":"        USED_IPS.update(ips)"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"class NoSuchQueue(Exception):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_816ceb37","line":166,"in_reply_to":"3a71b18c_a1e3a704","updated":"2016-12-07 21:43:57.000000000","message":"If this was just a refactor I would have left this out but it seems to be a refactor plus a bit. I meant to leave a note in the main comment box to the effect that these comments may or may not be applicable depending on what you considered in scope for the refactor.","commit_id":"ed75924eab87fdf0f10f5f417a885baf29401348"}]}