)]}'
{"nova/console/websocketproxy.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b5347a798c588ddfe595b458ca0721c0621c724c","unresolved":false,"context_lines":[{"line_number":195,"context_line":"        # Handshake as necessary"},{"line_number":196,"context_line":"        if connect_info.get(\u0027internal_access_path\u0027):"},{"line_number":197,"context_line":"            tsock.send(encodeutils.safe_encode("},{"line_number":198,"context_line":"                \"CONNECT %s HTTP/1.1\\r\\n\\r\\n\" %"},{"line_number":199,"context_line":"                connect_info[\u0027internal_access_path\u0027]))"},{"line_number":200,"context_line":"            end_token \u003d \"\\r\\n\\r\\n\""},{"line_number":201,"context_line":"            while True:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_64efbe87","line":198,"updated":"2018-01-10 17:03:10.000000000","message":"I can see that this made the py35 job pass in the DNM patch [1] but it\u0027s not clear to me how. The failure we were seeing was the code path added in [2] and I don\u0027t understand how this is related.\n\nPoint being, I\u0027d like to get the commit message clearly explaining what\u0027s wrong and why this fixes it.\n\nAlso, shouldn\u0027t this change be stacked earlier in the series? Basically, it should fix the problem *before* the code path gets enabled, not after.\n\n[1] https://review.openstack.org/#/c/530950\n[2] https://review.openstack.org/#/c/345398/29/nova/console/rfb/authvencrypt.py@68","commit_id":"6206058ffe2e6ad61d9792174cf98a8a253e1403"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"977834761bbc82430ef7b121f8ff6ddc486bcbb3","unresolved":false,"context_lines":[{"line_number":195,"context_line":"        # Handshake as necessary"},{"line_number":196,"context_line":"        if connect_info.get(\u0027internal_access_path\u0027):"},{"line_number":197,"context_line":"            tsock.send(encodeutils.safe_encode("},{"line_number":198,"context_line":"                \"CONNECT %s HTTP/1.1\\r\\n\\r\\n\" %"},{"line_number":199,"context_line":"                connect_info[\u0027internal_access_path\u0027]))"},{"line_number":200,"context_line":"            end_token \u003d \"\\r\\n\\r\\n\""},{"line_number":201,"context_line":"            while True:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_f3506135","line":198,"in_reply_to":"7f96bb07_64efbe87","updated":"2018-01-11 11:25:59.000000000","message":"\u003e I can see that this made the py35 job pass in the DNM patch [1] but\n \u003e it\u0027s not clear to me how. The failure we were seeing was the code\n \u003e path added in [2] and I don\u0027t understand how this is related.\n\nNope, this is nothing to do with [1]. [1] has a dependency on [3], which Depends-On [4] (a dependency of this patch). As such, this patch isn\u0027t tested at all by [1]. [1] is testing [4]. (yes, that was confusing :))\n\n \u003e Point being, I\u0027d like to get the commit message clearly explaining\n \u003e what\u0027s wrong and why this fixes it.\n\nI can do that. I\u0027d also like to add a test, but I don\u0027t know how I can do one in a meaningful way. Open to ideas here.\n\n \u003e Also, shouldn\u0027t this change be stacked earlier in the series?\n \u003e Basically, it should fix the problem *before* the code path gets\n \u003e enabled, not after.\n\nThat\u0027s probably correct. I left it at the end because it\u0027s a latent bug on \u0027master\u0027 that no one has spotted. I could actually apply this straight to master, if it would help?\n\n \u003e [1] https://review.openstack.org/#/c/530950\n \u003e [2] https://review.openstack.org/#/c/345398/29/nova/console/rfb/authvencrypt.py@68\n\n[3] https://review.openstack.org/#/c/396186/\n[4] https://review.openstack.org/#/c/345399/","commit_id":"6206058ffe2e6ad61d9792174cf98a8a253e1403"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"430c2b8f4820a3dd02f93319ded1af72b7596e26","unresolved":false,"context_lines":[{"line_number":195,"context_line":"        # Handshake as necessary"},{"line_number":196,"context_line":"        if connect_info.get(\u0027internal_access_path\u0027):"},{"line_number":197,"context_line":"            tsock.send(encodeutils.safe_encode("},{"line_number":198,"context_line":"                \"CONNECT %s HTTP/1.1\\r\\n\\r\\n\" %"},{"line_number":199,"context_line":"                connect_info[\u0027internal_access_path\u0027]))"},{"line_number":200,"context_line":"            end_token \u003d \"\\r\\n\\r\\n\""},{"line_number":201,"context_line":"            while True:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_37621540","line":198,"in_reply_to":"7f96bb07_88943f46","updated":"2018-01-12 09:42:36.000000000","message":"\u003e \u003e Nope, this is nothing to do with [1]. [1] has a dependency on [3], which Depends-On [4] (a dependency of this patch). As such, this patch isn\u0027t tested at all by [1]. [1] is testing [4]. (yes, that was confusing :))\n \u003e \n \u003e Oh, right. I got confused. So then, what made the py35 job start passing? You must have fixed something else in one of the patches? Sorry I lost track.\n\nIt was another, similarchange which I\u0027ve now merged in here [1].\n\n[1] https://review.openstack.org/#/c/531834/4/nova/console/websocketproxy.py@80\n\n \u003e \u003e I can do that. I\u0027d also like to add a test, but I don\u0027t know how I can do one in a meaningful way. Open to ideas here.\n \u003e \n \u003e Agreed, ideal to have a test but I also can\u0027t think of anything great there. Maybe a test that asserts the arg type passed to the send() call using MatchType?\n \u003e \n \u003e mock_send.assert_called_once_with(test.MatchType(bytes))\n\nThat\u0027ll do for now. Wish mypy was a practical thing to use here though :(","commit_id":"6206058ffe2e6ad61d9792174cf98a8a253e1403"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b1856f52a01196e3494ffb97741e3a3ff2b08d54","unresolved":false,"context_lines":[{"line_number":195,"context_line":"        # Handshake as necessary"},{"line_number":196,"context_line":"        if connect_info.get(\u0027internal_access_path\u0027):"},{"line_number":197,"context_line":"            tsock.send(encodeutils.safe_encode("},{"line_number":198,"context_line":"                \"CONNECT %s HTTP/1.1\\r\\n\\r\\n\" %"},{"line_number":199,"context_line":"                connect_info[\u0027internal_access_path\u0027]))"},{"line_number":200,"context_line":"            end_token \u003d \"\\r\\n\\r\\n\""},{"line_number":201,"context_line":"            while True:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_88943f46","line":198,"in_reply_to":"7f96bb07_f3506135","updated":"2018-01-11 18:52:09.000000000","message":"\u003e Nope, this is nothing to do with [1]. [1] has a dependency on [3],\n \u003e which Depends-On [4] (a dependency of this patch). As such, this\n \u003e patch isn\u0027t tested at all by [1]. [1] is testing [4]. (yes, that\n \u003e was confusing :))\n\nOh, right. I got confused. So then, what made the py35 job start passing? You must have fixed something else in one of the patches? Sorry I lost track.\n\n \u003e I can do that. I\u0027d also like to add a test, but I don\u0027t know how I\n \u003e can do one in a meaningful way. Open to ideas here.\n\nAgreed, ideal to have a test but I also can\u0027t think of anything great there. Maybe a test that asserts the arg type passed to the send() call using MatchType?\n\n mock_send.assert_called_once_with(test.MatchType(bytes))\n\n \u003e That\u0027s probably correct. I left it at the end because it\u0027s a latent\n \u003e bug on \u0027master\u0027 that no one has spotted. I could actually apply\n \u003e this straight to master, if it would help?\n\nI think what you did is right, stack it on the bottom of the series. Because if it\u0027s at the end and one of the patches in the middle enables the broken code path for an end user, we\u0027ll have an unnecessary window where things don\u0027t work under py35.","commit_id":"6206058ffe2e6ad61d9792174cf98a8a253e1403"}]}