)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bc60ec385f90f74fffd5ad7925d83ce9c55b61fe","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This should help the readability and also make it possible"},{"line_number":10,"context_line":"to call new _verify_and_write with different arguments."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: I20913201cf945a7fde1f9e6264c415e1235db7b9"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"bf51134e_e3924b4b","line":11,"updated":"2020-07-03 08:21:59.000000000","message":"You did one addition to the pulled out code, the new write_image parameter. When I asked for the refactoring I asked for it to only see the code moving around and maybe changing shape but don\u0027t changing the meaning. I think addition of the write_image param belongs to the patch implementing the actual feature change. But looking at the write_image here, now, I don\u0027t see why the new param is needed as the value of the data and dst_path params have already define when the image is written out or not.","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9963,"name":"Jiri Suchomel","email":"jiri.suchomel@suse.com","username":"jsuchome"},"change_message_id":"ed9c76c5f97fa0319e20cec2466038e64eb907b6","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This should help the readability and also make it possible"},{"line_number":10,"context_line":"to call new _verify_and_write with different arguments."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: I20913201cf945a7fde1f9e6264c415e1235db7b9"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"bf51134e_01893725","line":11,"in_reply_to":"bf51134e_e3924b4b","updated":"2020-07-03 11:03:26.000000000","message":"Hm, it\u0027s possible I had an iteration where the write_image was not passed but computed and it changed some tests results ... and I decided rather to keep the testsuite with minimal changes so it better shows nothing is broken....","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"}],"nova/image/glance.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bc60ec385f90f74fffd5ad7925d83ce9c55b61fe","unresolved":false,"context_lines":[{"line_number":335,"context_line":""},{"line_number":336,"context_line":"        Generally, this method just gets most of the arguments same as"},{"line_number":337,"context_line":"        the download method."},{"line_number":338,"context_line":""},{"line_number":339,"context_line":"        :param dst_path: Filepath to transfer the image file to."},{"line_number":340,"context_line":"        :param data: File object to use when writing the image."},{"line_number":341,"context_line":"            If passed as None and dst_path is provided, new file is opened."}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_03893f25","line":338,"updated":"2020-07-03 08:21:59.000000000","message":"I would add some more documentation:\n\nThis function writes the content of the image_chunks iterator either to a file object provided by the data param or to a filepath provided by dst_path param. If none of them are provided then no data will be written out but instead image_chunks iterator is returned.\n\n// This makes me feel that write_image param is a duplicate. As there is already a way to tell this function to write or not to write the image data out.","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9963,"name":"Jiri Suchomel","email":"jiri.suchomel@suse.com","username":"jsuchome"},"change_message_id":"ed9c76c5f97fa0319e20cec2466038e64eb907b6","unresolved":false,"context_lines":[{"line_number":335,"context_line":""},{"line_number":336,"context_line":"        Generally, this method just gets most of the arguments same as"},{"line_number":337,"context_line":"        the download method."},{"line_number":338,"context_line":""},{"line_number":339,"context_line":"        :param dst_path: Filepath to transfer the image file to."},{"line_number":340,"context_line":"        :param data: File object to use when writing the image."},{"line_number":341,"context_line":"            If passed as None and dst_path is provided, new file is opened."}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_a1d68bf0","line":338,"in_reply_to":"bf51134e_03893f25","updated":"2020-07-03 11:03:26.000000000","message":"Good one.","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bc60ec385f90f74fffd5ad7925d83ce9c55b61fe","unresolved":false,"context_lines":[{"line_number":337,"context_line":"        the download method."},{"line_number":338,"context_line":""},{"line_number":339,"context_line":"        :param dst_path: Filepath to transfer the image file to."},{"line_number":340,"context_line":"        :param data: File object to use when writing the image."},{"line_number":341,"context_line":"            If passed as None and dst_path is provided, new file is opened."},{"line_number":342,"context_line":"        :param write_image: Write the image file provided by image_chunks."},{"line_number":343,"context_line":"        :returns an iterable with image data, or nothing. Iterable is returned"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_035f7fe5","line":340,"range":{"start_line":340,"start_character":21,"end_line":340,"end_character":63},"updated":"2020-07-03 08:21:59.000000000","message":"Does write_image param influences this behavior as well?","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bc60ec385f90f74fffd5ad7925d83ce9c55b61fe","unresolved":false,"context_lines":[{"line_number":337,"context_line":"        the download method."},{"line_number":338,"context_line":""},{"line_number":339,"context_line":"        :param dst_path: Filepath to transfer the image file to."},{"line_number":340,"context_line":"        :param data: File object to use when writing the image."},{"line_number":341,"context_line":"            If passed as None and dst_path is provided, new file is opened."},{"line_number":342,"context_line":"        :param write_image: Write the image file provided by image_chunks."},{"line_number":343,"context_line":"        :returns an iterable with image data, or nothing. Iterable is returned"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_8326ef15","line":340,"range":{"start_line":340,"start_character":15,"end_line":340,"end_character":19},"updated":"2020-07-03 08:21:59.000000000","message":"I would love to rename \u0027data\u0027 to something meaningful. Like dst_file","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9963,"name":"Jiri Suchomel","email":"jiri.suchomel@suse.com","username":"jsuchome"},"change_message_id":"ed9c76c5f97fa0319e20cec2466038e64eb907b6","unresolved":false,"context_lines":[{"line_number":337,"context_line":"        the download method."},{"line_number":338,"context_line":""},{"line_number":339,"context_line":"        :param dst_path: Filepath to transfer the image file to."},{"line_number":340,"context_line":"        :param data: File object to use when writing the image."},{"line_number":341,"context_line":"            If passed as None and dst_path is provided, new file is opened."},{"line_number":342,"context_line":"        :param write_image: Write the image file provided by image_chunks."},{"line_number":343,"context_line":"        :returns an iterable with image data, or nothing. Iterable is returned"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_61cc7361","line":340,"range":{"start_line":340,"start_character":15,"end_line":340,"end_character":19},"in_reply_to":"bf51134e_8326ef15","updated":"2020-07-03 11:03:26.000000000","message":"Yeah, those are ugly. But those are the same args that are already passed to download method...","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bc60ec385f90f74fffd5ad7925d83ce9c55b61fe","unresolved":false,"context_lines":[{"line_number":339,"context_line":"        :param dst_path: Filepath to transfer the image file to."},{"line_number":340,"context_line":"        :param data: File object to use when writing the image."},{"line_number":341,"context_line":"            If passed as None and dst_path is provided, new file is opened."},{"line_number":342,"context_line":"        :param write_image: Write the image file provided by image_chunks."},{"line_number":343,"context_line":"        :returns an iterable with image data, or nothing. Iterable is returned"},{"line_number":344,"context_line":"            only when data param is None and dst_path is not provided (assuming"},{"line_number":345,"context_line":"            the caller wants to process the data by itself)."}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_63cbdb71","line":342,"updated":"2020-07-03 08:21:59.000000000","message":"please specify the rest of the params too.","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bc60ec385f90f74fffd5ad7925d83ce9c55b61fe","unresolved":false,"context_lines":[{"line_number":351,"context_line":"            data \u003d open(dst_path, \u0027wb\u0027)"},{"line_number":352,"context_line":"            close_file \u003d True"},{"line_number":353,"context_line":""},{"line_number":354,"context_line":"        if data is None:"},{"line_number":355,"context_line":"            write_image \u003d False"},{"line_number":356,"context_line":""},{"line_number":357,"context_line":"        # Retrieve properties for verification of Glance image signature"},{"line_number":358,"context_line":"        verifier \u003d self._get_verifier(context, image_id, trusted_certs)"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_e3a40bbc","line":355,"range":{"start_line":354,"start_character":1,"end_line":355,"end_character":31},"updated":"2020-07-03 08:21:59.000000000","message":"Assuming we were called with write_image\u003dTrue. Then it seems strange that the caller asked for writing the image out to the disk but did not provided the necessary information to do so (e.g. either dst_path or an open file object to write to). I would say the caller made a mistake so this is an error case here.","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9963,"name":"Jiri Suchomel","email":"jiri.suchomel@suse.com","username":"jsuchome"},"change_message_id":"ed9c76c5f97fa0319e20cec2466038e64eb907b6","unresolved":false,"context_lines":[{"line_number":351,"context_line":"            data \u003d open(dst_path, \u0027wb\u0027)"},{"line_number":352,"context_line":"            close_file \u003d True"},{"line_number":353,"context_line":""},{"line_number":354,"context_line":"        if data is None:"},{"line_number":355,"context_line":"            write_image \u003d False"},{"line_number":356,"context_line":""},{"line_number":357,"context_line":"        # Retrieve properties for verification of Glance image signature"},{"line_number":358,"context_line":"        verifier \u003d self._get_verifier(context, image_id, trusted_certs)"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_21043b7b","line":355,"range":{"start_line":354,"start_character":1,"end_line":355,"end_character":31},"in_reply_to":"bf51134e_e3a40bbc","updated":"2020-07-03 11:03:26.000000000","message":"I\u0027ll try with removing write_image param completely","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bc60ec385f90f74fffd5ad7925d83ce9c55b61fe","unresolved":false,"context_lines":[{"line_number":381,"context_line":"                LOG.error(\u0027Image signature verification failed \u0027"},{"line_number":382,"context_line":"                          \u0027for image %s\u0027, image_id)"},{"line_number":383,"context_line":"        except Exception as ex:"},{"line_number":384,"context_line":"            if write_image:"},{"line_number":385,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":386,"context_line":"                    LOG.error(\"Error writing to %(path)s: %(exception)s\","},{"line_number":387,"context_line":"                              {\u0027path\u0027: dst_path, \u0027exception\u0027: ex})"},{"line_number":388,"context_line":"        finally:"},{"line_number":389,"context_line":"            if close_file:"},{"line_number":390,"context_line":"                # Ensure that the data is pushed all the way down to"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_2313a30f","line":387,"range":{"start_line":384,"start_character":0,"end_line":387,"end_character":66},"updated":"2020-07-03 08:21:59.000000000","message":"This could potentially silently catch and ignore an exception if write_image is False.","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9963,"name":"Jiri Suchomel","email":"jiri.suchomel@suse.com","username":"jsuchome"},"change_message_id":"ed9c76c5f97fa0319e20cec2466038e64eb907b6","unresolved":false,"context_lines":[{"line_number":381,"context_line":"                LOG.error(\u0027Image signature verification failed \u0027"},{"line_number":382,"context_line":"                          \u0027for image %s\u0027, image_id)"},{"line_number":383,"context_line":"        except Exception as ex:"},{"line_number":384,"context_line":"            if write_image:"},{"line_number":385,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":386,"context_line":"                    LOG.error(\"Error writing to %(path)s: %(exception)s\","},{"line_number":387,"context_line":"                              {\u0027path\u0027: dst_path, \u0027exception\u0027: ex})"},{"line_number":388,"context_line":"        finally:"},{"line_number":389,"context_line":"            if close_file:"},{"line_number":390,"context_line":"                # Ensure that the data is pushed all the way down to"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_a1efabae","line":387,"range":{"start_line":384,"start_character":0,"end_line":387,"end_character":66},"in_reply_to":"bf51134e_2313a30f","updated":"2020-07-03 11:03:26.000000000","message":"That\u0027s true ... but that\u0027s nothing new compared to previous state I think","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5ea24a95ad8b22ff3e84241d313f0e573477bf9b","unresolved":false,"context_lines":[{"line_number":381,"context_line":"                LOG.error(\u0027Image signature verification failed \u0027"},{"line_number":382,"context_line":"                          \u0027for image %s\u0027, image_id)"},{"line_number":383,"context_line":"        except Exception as ex:"},{"line_number":384,"context_line":"            if write_image:"},{"line_number":385,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":386,"context_line":"                    LOG.error(\"Error writing to %(path)s: %(exception)s\","},{"line_number":387,"context_line":"                              {\u0027path\u0027: dst_path, \u0027exception\u0027: ex})"},{"line_number":388,"context_line":"        finally:"},{"line_number":389,"context_line":"            if close_file:"},{"line_number":390,"context_line":"                # Ensure that the data is pushed all the way down to"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_f0766406","line":387,"range":{"start_line":384,"start_character":0,"end_line":387,"end_character":66},"in_reply_to":"bf51134e_a1efabae","updated":"2020-07-07 12:30:36.000000000","message":"In the previous state there was no guard condition in the except block so the exception was always reraised by the save_and_reraise_exception() context manager. Now if write_image is False then the exception is silently dropped.","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9963,"name":"Jiri Suchomel","email":"jiri.suchomel@suse.com","username":"jsuchome"},"change_message_id":"b0c24f2bbf05131a9da7dead2b026bb3d1f78208","unresolved":false,"context_lines":[{"line_number":381,"context_line":"                LOG.error(\u0027Image signature verification failed \u0027"},{"line_number":382,"context_line":"                          \u0027for image %s\u0027, image_id)"},{"line_number":383,"context_line":"        except Exception as ex:"},{"line_number":384,"context_line":"            if write_image:"},{"line_number":385,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":386,"context_line":"                    LOG.error(\"Error writing to %(path)s: %(exception)s\","},{"line_number":387,"context_line":"                              {\u0027path\u0027: dst_path, \u0027exception\u0027: ex})"},{"line_number":388,"context_line":"        finally:"},{"line_number":389,"context_line":"            if close_file:"},{"line_number":390,"context_line":"                # Ensure that the data is pushed all the way down to"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_b60befd2","line":387,"range":{"start_line":384,"start_character":0,"end_line":387,"end_character":66},"in_reply_to":"bf51134e_f0766406","updated":"2020-07-13 13:54:25.000000000","message":"OK, that\u0027s true (although the exception that was raised in that case was not write related but the message said it was).\nI\u0027ll add a branch for some generic error...","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"bc60ec385f90f74fffd5ad7925d83ce9c55b61fe","unresolved":false,"context_lines":[{"line_number":395,"context_line":"                self._safe_fsync(data)"},{"line_number":396,"context_line":"                data.close()"},{"line_number":397,"context_line":""},{"line_number":398,"context_line":"        if data is None:"},{"line_number":399,"context_line":"            return image_chunks"},{"line_number":400,"context_line":""},{"line_number":401,"context_line":"    def _get_verifier(self, context, image_id, trusted_certs):"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_43ffb766","line":398,"range":{"start_line":398,"start_character":0,"end_line":398,"end_character":24},"updated":"2020-07-03 08:21:59.000000000","message":"Is it write_image or data param that should really drive  when this function returns image_chunks?\n\nAs far as I understand we want to return the image_chunks iterator to allow the caller to process the image data. We can only do that if we do not consumed the iterator. We consume the iterator:\n* if we do verification or\n* if we write image out to data file obj or to dst_path","commit_id":"a05a30e88cf354ea63ab22b30e267fd2626e5a79"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"5ea24a95ad8b22ff3e84241d313f0e573477bf9b","unresolved":false,"context_lines":[{"line_number":403,"context_line":"                data.close()"},{"line_number":404,"context_line":""},{"line_number":405,"context_line":"        if data is None:"},{"line_number":406,"context_line":"            return image_chunks"},{"line_number":407,"context_line":""},{"line_number":408,"context_line":"    def _get_verifier(self, context, image_id, trusted_certs):"},{"line_number":409,"context_line":"        verifier \u003d None"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_b0da2ce6","line":406,"updated":"2020-07-07 12:30:36.000000000","message":"There is a hidden bug here which was possibly exists in the original code too. \nIf there is no data or dst_path is provided then no data will be written out and the image_chunk is returned. But if there is verifier returned by  _get_verifier() then this function consumes the image_chunk iterator for verification and then returns a consumed iterator. So the caller cannot do the data copying later.\n\nAs this is unrelated to your current patch series lets have this fixed in a totally separate patch.","commit_id":"6d7688110003fa24fe554d187664c94e0702a3d7"},{"author":{"_account_id":9963,"name":"Jiri Suchomel","email":"jiri.suchomel@suse.com","username":"jsuchome"},"change_message_id":"b0c24f2bbf05131a9da7dead2b026bb3d1f78208","unresolved":false,"context_lines":[{"line_number":403,"context_line":"                data.close()"},{"line_number":404,"context_line":""},{"line_number":405,"context_line":"        if data is None:"},{"line_number":406,"context_line":"            return image_chunks"},{"line_number":407,"context_line":""},{"line_number":408,"context_line":"    def _get_verifier(self, context, image_id, trusted_certs):"},{"line_number":409,"context_line":"        verifier \u003d None"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_36c6bf69","line":406,"in_reply_to":"bf51134e_b0da2ce6","updated":"2020-07-13 13:54:25.000000000","message":"Yeah, I\u0027m not sure what\u0027s the best way to approach that...","commit_id":"6d7688110003fa24fe554d187664c94e0702a3d7"}]}