)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"84a11938276fca3537cb91e6fc62a4e5aaf03197","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"1cec8087_f0c1d8ac","updated":"2024-08-01 11:33:25.000000000","message":"Also submitted for Glance: https://review.opendev.org/c/openstack/glance/+/925466","commit_id":"abe76ef198111b5997d634f69ce780c32d2ca834"}],"cinder/tests/unit/image/test_format_inspector.py":[{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"25a1720735c4f969a578a19f23f61ae0ecfd46b8","unresolved":true,"context_lines":[{"line_number":179,"context_line":"        # a local file."},{"line_number":180,"context_line":"        self.assertLess(fmt.actual_size, file_size)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"    def qed_supported(self):"},{"line_number":183,"context_line":"        output \u003d subprocess.check_output(\u0027qemu-img create --help\u0027, shell\u003dTrue)"},{"line_number":184,"context_line":"        return b\u0027 qed \u0027 in output"},{"line_number":185,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"7f90e701_0416d3bb","line":182,"updated":"2025-08-07 15:19:25.000000000","message":"nit: I think this is meant to be private, function could start with an underscore.","commit_id":"abe76ef198111b5997d634f69ce780c32d2ca834"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"63a6315598893ef50aab6e83d33a3415f170db3d","unresolved":true,"context_lines":[{"line_number":179,"context_line":"        # a local file."},{"line_number":180,"context_line":"        self.assertLess(fmt.actual_size, file_size)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"    def qed_supported(self):"},{"line_number":183,"context_line":"        output \u003d subprocess.check_output(\u0027qemu-img create --help\u0027, shell\u003dTrue)"},{"line_number":184,"context_line":"        return b\u0027 qed \u0027 in output"},{"line_number":185,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"dcf0adeb_81ce2a1a","line":182,"in_reply_to":"668befec_db54698e","updated":"2026-08-27 17:40:19.000000000","message":"This already happened in Patchset 2?","commit_id":"abe76ef198111b5997d634f69ce780c32d2ca834"},{"author":{"_account_id":10058,"name":"Erlon R. Cruz","email":"erlon.rodrigues.cruz@canonical.com","username":"sombrafam"},"change_message_id":"0cbb4e5a4e5471a5ffb443a5a6aa74f8fb779ea4","unresolved":true,"context_lines":[{"line_number":179,"context_line":"        # a local file."},{"line_number":180,"context_line":"        self.assertLess(fmt.actual_size, file_size)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"    def qed_supported(self):"},{"line_number":183,"context_line":"        output \u003d subprocess.check_output(\u0027qemu-img create --help\u0027, shell\u003dTrue)"},{"line_number":184,"context_line":"        return b\u0027 qed \u0027 in output"},{"line_number":185,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ff4b87c7_cf80e86d","line":182,"in_reply_to":"7f90e701_0416d3bb","updated":"2026-07-29 14:24:46.000000000","message":"-1: I actually think that flagging this as private is important, because it saves us time looking through the code. \nAnother point I have is, shouldn\u0027t unit tests be using fixtures instead of calling outside commands?","commit_id":"abe76ef198111b5997d634f69ce780c32d2ca834"},{"author":{"_account_id":10058,"name":"Erlon R. Cruz","email":"erlon.rodrigues.cruz@canonical.com","username":"sombrafam"},"change_message_id":"f4b1feec0f99216b142145ed78fe7fb2dbcca26b","unresolved":true,"context_lines":[{"line_number":179,"context_line":"        # a local file."},{"line_number":180,"context_line":"        self.assertLess(fmt.actual_size, file_size)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"    def qed_supported(self):"},{"line_number":183,"context_line":"        output \u003d subprocess.check_output(\u0027qemu-img create --help\u0027, shell\u003dTrue)"},{"line_number":184,"context_line":"        return b\u0027 qed \u0027 in output"},{"line_number":185,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"668befec_db54698e","line":182,"in_reply_to":"b75b7ab0_ec0e9f20","updated":"2026-08-26 14:37:29.000000000","message":"Got it, now I see that it\u0027s using the binary to actually create an image. I think it\u0027s better, not having a blob in the tests, as it could add security risks as well make patches hard to read. \nIll kepp the -1 for the internal function _re-naming","commit_id":"abe76ef198111b5997d634f69ce780c32d2ca834"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"b90b44e7b90a8df280b117bc9312ff89853dd9b9","unresolved":true,"context_lines":[{"line_number":179,"context_line":"        # a local file."},{"line_number":180,"context_line":"        self.assertLess(fmt.actual_size, file_size)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"    def qed_supported(self):"},{"line_number":183,"context_line":"        output \u003d subprocess.check_output(\u0027qemu-img create --help\u0027, shell\u003dTrue)"},{"line_number":184,"context_line":"        return b\u0027 qed \u0027 in output"},{"line_number":185,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"b75b7ab0_ec0e9f20","line":182,"in_reply_to":"ff4b87c7_cf80e86d","updated":"2026-07-30 14:05:09.000000000","message":"I\u0027m not really sure what our standard policy is for private methods in unit tests, but I can update it.\n\n\u003e Another point I have is, shouldn\u0027t unit tests be using fixtures instead of calling outside commands?\n\nIdeally, but for these tests we decided it\u0027d be easier to use qemu-img to generate images needed for tests rather than carrying that data in the test suite. Could be something to improve in the future.","commit_id":"abe76ef198111b5997d634f69ce780c32d2ca834"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a07cab75f13b0af50fa6bc04a080815193ed63b1","unresolved":false,"context_lines":[{"line_number":180,"context_line":"        self.assertLess(fmt.actual_size, file_size)"},{"line_number":181,"context_line":""},{"line_number":182,"context_line":"    def _qed_supported(self):"},{"line_number":183,"context_line":"        output \u003d subprocess.check_output(\u0027qemu-img create --help\u0027, shell\u003dTrue)"},{"line_number":184,"context_line":"        return b\u0027 qed \u0027 in output"},{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def test_qed_always_unsafe(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"beeff489_58ac8f40","line":183,"range":{"start_line":183,"start_character":67,"end_line":183,"end_character":77},"updated":"2026-08-03 09:18:30.000000000","message":"so we need to pass shell\u003dTrue.\n\nelse\n```\n\u003e\u003e\u003e subprocess.check_output(\u0027qemu-img create --help\u0027)\nTraceback (most recent call last):\n  File \"\u003cpython-input-2\u003e\", line 1, in \u003cmodule\u003e\n    subprocess.check_output(\u0027qemu-img create --help\u0027)\n    ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/lib64/python3.14/subprocess.py\", line 473, in check_output\n    return run(*popenargs, stdout\u003dPIPE, timeout\u003dtimeout, check\u003dTrue,\n           ~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n               **kwargs).stdout\n               ^^^^^^^^^\n  File \"/usr/lib64/python3.14/subprocess.py\", line 555, in run\n    with Popen(*popenargs, **kwargs) as process:\n         ~~~~~^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/lib64/python3.14/subprocess.py\", line 1039, in __init__\n    self._execute_child(args, executable, preexec_fn, close_fds,\n    ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n                        pass_fds, cwd, env,\n                        ^^^^^^^^^^^^^^^^^^^\n    ...\u003c5 lines\u003e...\n                        gid, gids, uid, umask,\n                        ^^^^^^^^^^^^^^^^^^^^^^\n                        start_new_session, process_group)\n                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/lib64/python3.14/subprocess.py\", line 1990, in _execute_child\n    raise child_exception_type(errno_num, err_msg, err_filename)\nFileNotFoundError: [Errno 2] No such file or directory: \u0027qemu-img create --help\u0027\n```","commit_id":"22500a528435f021dcc756178b91b2bcfb0aec61"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"a07cab75f13b0af50fa6bc04a080815193ed63b1","unresolved":true,"context_lines":[{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def test_qed_always_unsafe(self):"},{"line_number":187,"context_line":"        if not self._qed_supported():"},{"line_number":188,"context_line":"            raise test.testtools.TestCase.skipException("},{"line_number":189,"context_line":"                \u0027qed not supported by qemu-img\u0027)"},{"line_number":190,"context_line":"        img \u003d self._create_img(\u0027qed\u0027, 10 * units.Mi)"},{"line_number":191,"context_line":"        fmt \u003d format_inspector.get_inspector(\u0027qed\u0027).from_file(img)"}],"source_content_type":"text/x-python","patch_set":2,"id":"d1be5440_3795b01c","line":188,"range":{"start_line":188,"start_character":12,"end_line":188,"end_character":17},"updated":"2026-08-03 09:18:30.000000000","message":"why raise exception, this will be called tox -e py, so pytest. should we use self.skipTest instead ?","commit_id":"22500a528435f021dcc756178b91b2bcfb0aec61"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"7d191a16b517285d75acad6adda158ad9433161e","unresolved":true,"context_lines":[{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def test_qed_always_unsafe(self):"},{"line_number":187,"context_line":"        if not self._qed_supported():"},{"line_number":188,"context_line":"            raise test.testtools.TestCase.skipException("},{"line_number":189,"context_line":"                \u0027qed not supported by qemu-img\u0027)"},{"line_number":190,"context_line":"        img \u003d self._create_img(\u0027qed\u0027, 10 * units.Mi)"},{"line_number":191,"context_line":"        fmt \u003d format_inspector.get_inspector(\u0027qed\u0027).from_file(img)"}],"source_content_type":"text/x-python","patch_set":2,"id":"4311292b_8ec0ea89","line":188,"range":{"start_line":188,"start_character":12,"end_line":188,"end_character":17},"in_reply_to":"794d484f_c6d7d875","updated":"2026-08-04 03:56:16.000000000","message":"yeah this works too, also as CI is happy so no objection from me.\njust a minor preference, `self.skipTest()` is the more common/standard pattern, so it might be worth considering for readability. but not a blocker.","commit_id":"22500a528435f021dcc756178b91b2bcfb0aec61"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"9aca45cedc2277ccef36d81f1bde15f05f42934e","unresolved":true,"context_lines":[{"line_number":185,"context_line":""},{"line_number":186,"context_line":"    def test_qed_always_unsafe(self):"},{"line_number":187,"context_line":"        if not self._qed_supported():"},{"line_number":188,"context_line":"            raise test.testtools.TestCase.skipException("},{"line_number":189,"context_line":"                \u0027qed not supported by qemu-img\u0027)"},{"line_number":190,"context_line":"        img \u003d self._create_img(\u0027qed\u0027, 10 * units.Mi)"},{"line_number":191,"context_line":"        fmt \u003d format_inspector.get_inspector(\u0027qed\u0027).from_file(img)"}],"source_content_type":"text/x-python","patch_set":2,"id":"794d484f_c6d7d875","line":188,"range":{"start_line":188,"start_character":12,"end_line":188,"end_character":17},"in_reply_to":"d1be5440_3795b01c","updated":"2026-08-03 14:16:57.000000000","message":"This exception skips the test.\n\nYes, we could probably use self instead of test.testtools.TestCase.\n\nSee also, this patch that merged two years ago: https://review.opendev.org/c/openstack/glance/+/925466","commit_id":"22500a528435f021dcc756178b91b2bcfb0aec61"}]}