)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"004aa78fbfe703d67e766ef1089800dde6997d5e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3cd49bb8_7c225f1f","updated":"2025-08-18 13:02:23.000000000","message":"I\u0027m not sure about this one. This is a very admin-specific command that I wouldn\u0027t expect to be called often, and the presence of all the conditionals in the test suggest much of this would never be executed.\n\nWhen you run this locally, do you see cached images appearing? If so, why do we need all the conditionals in the test?","commit_id":"68daf90815b8929a362794df1f997b4ad0e1e870"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"fc696bc5b3d7ab709f5b0ec5e14895704e380e08","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"50176f2e_14acada8","in_reply_to":"3cd49bb8_7c225f1f","updated":"2025-08-20 14:14:39.000000000","message":"Thank you for the thoughtful code review.\n\nCurrent behavior (verified locally)\nWhen I run the tests locally, cached images do appear. However, the cache transition happens very quickly, so the image moves from queued → cached almost immediately.\n\n```bash\n$ openstack cached image queue \u003cimage-id\u003e\n$ openstack cached image list\n+--------------------------------------+--------+---------------------+---------------------+---------+------+\n| ID                                   | State  | Last Accessed (UTC) | Last Modified (UTC) |    Size | Hits |\n+--------------------------------------+--------+---------------------+---------------------+---------+------+\n| 498f4607-8adb-4822-8c3d-cfbc48614571 | cached | 2025-08-20T03:12:25 | 2025-08-20T03:12:25 | 1048576 |    0 |\n+--------------------------------------+--------+---------------------+---------------------+---------+------+\n```\n\nWhy the conditionals exist (and current limitations)\n\nDifficulty testing --queue: I don’t have a reliable way to keep the status fixed at queued, which makes a stable test hard to write.\n\nEnvironment variability: Cache speed and initial cache state differ by environment.\n\nDefensive coding: The guards were added to reduce flakiness in CI.\n\nProposed path forward\nGiven that I can’t currently pin the status to queued, would it be acceptable to land the tests that don’t assert --queue semantics first and handle --queue separately? I’m also open to alternatives (e.g., introducing a controllable delay/mocking the cache backend) if you prefer.\n\nI’d appreciate your guidance on which direction you’d like me to take. Thank you again!","commit_id":"68daf90815b8929a362794df1f997b4ad0e1e870"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"2ef200b1af3db1e157d3f0b2e54bac687219c6d4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"aedd524d_e0e2a37f","in_reply_to":"50176f2e_14acada8","updated":"2025-08-20 17:01:02.000000000","message":"Given we can\u0027t make this deterministic, I am inclined not to add those tests. Rather than adding 3 separate tests, could we rename `test_cache_clear_both` to `test_cached_image` and delete the other two tests? You should also move the contents of the `setUp` and `tearDown` into this test","commit_id":"68daf90815b8929a362794df1f997b4ad0e1e870"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"5e641e97319258011e73e653d7d7cb79ab3d6f49","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"166487fd_674e3b56","in_reply_to":"aedd524d_e0e2a37f","updated":"2025-08-21 05:55:34.000000000","message":"Thanks for the guidance. \n\nI’ll follow this: rename test_cache_clear_both to test_cached_image, remove the other two tests, and inline setUp/tearDown into that single test (with try/finally). I’ll avoid global state/global flags and ensure linters/tests pass before resubmitting.","commit_id":"68daf90815b8929a362794df1f997b4ad0e1e870"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"db5f0fdfd36b1362c3894ec3adbab97d6e6cbb07","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"86b6c4bb_22911a16","updated":"2025-09-09 21:51:09.000000000","message":"This looks like a duplicate of https://review.opendev.org/c/openstack/python-openstackclient/+/958937. I haven\u0027t reviewed it yet. Can you please figure out which version you\u0027d like to go with and close the other one?","commit_id":"7ad7fd9b74aad1f6fbcbc4a9b964a429afbd4f0d"},{"author":{"_account_id":35119,"name":"jihyun huh","email":"huhji.elha@gmail.com","username":"jhhuh"},"change_message_id":"b9ab1742fd4128d6af55dbfc99ce6be3c78f3bef","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"d5bc2ba5_98d6d73d","updated":"2025-08-26 12:10:07.000000000","message":"recheck","commit_id":"7ad7fd9b74aad1f6fbcbc4a9b964a429afbd4f0d"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"6a0e18c1a30c036f0be19cd5b16d558471c38017","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"3798a0c7_ad7c8ea6","in_reply_to":"86b6c4bb_22911a16","updated":"2025-09-10 10:57:55.000000000","message":"Thanks for the note. The author of the linked change (958937) has agreed to close it, and I’ll proceed with my current commit. Please let me know if you’d prefer the other version instead.","commit_id":"7ad7fd9b74aad1f6fbcbc4a9b964a429afbd4f0d"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"d149cc22ec90a73bbda1fde72a8e6e411e9dd940","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"9d22ea03_3010b534","updated":"2025-11-13 09:50:00.000000000","message":"recheck","commit_id":"4cf70113d2398e432bae4ba897de15d8f93615b2"}],"openstackclient/tests/functional/image/v2/test_cache.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"89ca46c7da904a9592e49f694a37a89413ddc383","unresolved":true,"context_lines":[{"line_number":23,"context_line":"    def setUp(self):"},{"line_number":24,"context_line":"        super().setUp()"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"        ver_fixture \u003d fixtures.EnvironmentVariable(\u0027OS_IMAGE_API_VERSION\u0027, \u00272\u0027)"},{"line_number":27,"context_line":"        self.useFixture(ver_fixture)"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"        self.name \u003d uuid.uuid4().hex"}],"source_content_type":"text/x-python","patch_set":2,"id":"400ffefe_1453ec48","line":26,"updated":"2025-08-18 12:59:41.000000000","message":"nit: While sensible, you likely don\u0027t need this since v1 is ancient and should rarely if ever be used.","commit_id":"68daf90815b8929a362794df1f997b4ad0e1e870"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"5e641e97319258011e73e653d7d7cb79ab3d6f49","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    def setUp(self):"},{"line_number":24,"context_line":"        super().setUp()"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"        ver_fixture \u003d fixtures.EnvironmentVariable(\u0027OS_IMAGE_API_VERSION\u0027, \u00272\u0027)"},{"line_number":27,"context_line":"        self.useFixture(ver_fixture)"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"        self.name \u003d uuid.uuid4().hex"}],"source_content_type":"text/x-python","patch_set":2,"id":"e2352fb1_9e469cf2","line":26,"in_reply_to":"400ffefe_1453ec48","updated":"2025-08-21 05:55:34.000000000","message":"Acknowledged","commit_id":"68daf90815b8929a362794df1f997b4ad0e1e870"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"93545a22d50ce6ed0f31770fe467362c9ab09f62","unresolved":true,"context_lines":[{"line_number":60,"context_line":"            img for img in final_output if img.get(\u0027State\u0027) \u003d\u003d \u0027queued\u0027"},{"line_number":61,"context_line":"        ]"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"        if final_queued_images:"},{"line_number":64,"context_line":"            queued_image_ids \u003d [img[\u0027ID\u0027] for img in final_queued_images]"},{"line_number":65,"context_line":"            self.assertNotIn(self.image_id, queued_image_ids)"},{"line_number":66,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1eb14a30_bcb9e09d","line":63,"updated":"2025-08-18 13:03:01.000000000","message":"Will this ever be true? If so, will it ever be false? See my top-level comment about the conditionals in this test.","commit_id":"68daf90815b8929a362794df1f997b4ad0e1e870"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"5e641e97319258011e73e653d7d7cb79ab3d6f49","unresolved":false,"context_lines":[{"line_number":60,"context_line":"            img for img in final_output if img.get(\u0027State\u0027) \u003d\u003d \u0027queued\u0027"},{"line_number":61,"context_line":"        ]"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"        if final_queued_images:"},{"line_number":64,"context_line":"            queued_image_ids \u003d [img[\u0027ID\u0027] for img in final_queued_images]"},{"line_number":65,"context_line":"            self.assertNotIn(self.image_id, queued_image_ids)"},{"line_number":66,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"8331c423_7b9f1108","line":63,"in_reply_to":"1eb14a30_bcb9e09d","updated":"2025-08-21 05:55:34.000000000","message":"Acknowledged","commit_id":"68daf90815b8929a362794df1f997b4ad0e1e870"},{"author":{"_account_id":35119,"name":"jihyun huh","email":"huhji.elha@gmail.com","username":"jhhuh"},"change_message_id":"60e1c19c20cfc2016b6c252b767678c35a66fefb","unresolved":true,"context_lines":[{"line_number":28,"context_line":"        image_id \u003d output[\"id\"]"},{"line_number":29,"context_line":"        self.assertOutput(name, output[\u0027name\u0027])"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"        try:"},{"line_number":32,"context_line":"            self.openstack(\u0027cached image queue \u0027 + image_id)"},{"line_number":33,"context_line":"            self.openstack(\u0027cached image clear\u0027)"},{"line_number":34,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"88ca7b44_45f35fea","line":31,"updated":"2025-08-23 06:31:28.000000000","message":"can i ask the reason you used `try` method twice in here? cause this `test_cached_images` function including 1. image create 2. cached image queue 3. cached image list 4. cached image clear 5. cached image delete\n\nand it would be seems more readable to sepeate each method in one function with code comment like this:\nhttps://opendev.org/openstack/python-openstackclient/src/branch/master/openstackclient/tests/functional/identity/v3/test_access_rule.py\nhttps://opendev.org/openstack/openstacksdk/src/branch/master/openstack/tests/functional/identity/v3/test_limit.py","commit_id":"6b15f6a779b8723e53f49befec523f6c93e47acf"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"b61cec85f0ae3f92ee3af5412fb7deafb4b823e2","unresolved":false,"context_lines":[{"line_number":28,"context_line":"        image_id \u003d output[\"id\"]"},{"line_number":29,"context_line":"        self.assertOutput(name, output[\u0027name\u0027])"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"        try:"},{"line_number":32,"context_line":"            self.openstack(\u0027cached image queue \u0027 + image_id)"},{"line_number":33,"context_line":"            self.openstack(\u0027cached image clear\u0027)"},{"line_number":34,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3f8a9521_ed7dee97","line":31,"in_reply_to":"391116c3_6e80d6d3","updated":"2025-08-26 04:51:52.000000000","message":"Done","commit_id":"6b15f6a779b8723e53f49befec523f6c93e47acf"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"78e7422917cdc1cd614f50d2995f04c6c2645875","unresolved":true,"context_lines":[{"line_number":28,"context_line":"        image_id \u003d output[\"id\"]"},{"line_number":29,"context_line":"        self.assertOutput(name, output[\u0027name\u0027])"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"        try:"},{"line_number":32,"context_line":"            self.openstack(\u0027cached image queue \u0027 + image_id)"},{"line_number":33,"context_line":"            self.openstack(\u0027cached image clear\u0027)"},{"line_number":34,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"391116c3_6e80d6d3","line":31,"in_reply_to":"88ca7b44_45f35fea","updated":"2025-08-26 03:30:41.000000000","message":"Thank you for the review. \n\nI used nested try blocks to ensure exception-safe cleanup—so that image delete will always run even if cached image delete fails. \n\nI appreciate the feedback on readability and will refactor to improve clarity. Thank you.","commit_id":"6b15f6a779b8723e53f49befec523f6c93e47acf"},{"author":{"_account_id":35119,"name":"jihyun huh","email":"huhji.elha@gmail.com","username":"jhhuh"},"change_message_id":"7740243d0d4a2cf5e0fe08262941b5e227619ab8","unresolved":true,"context_lines":[{"line_number":36,"context_line":"        self.addCleanup(self.openstack, \u0027image delete \u0027 + image_id)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        # Queue image for caching"},{"line_number":39,"context_line":"        self.openstack(\u0027cached image queue \u0027 + image_id)"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # Clear cached images"},{"line_number":42,"context_line":"        self.openstack(\u0027cached image clear\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"d9e092ce_0e85b5b2","line":39,"updated":"2025-09-11 10:16:55.000000000","message":"Can you add verification of the queued image after \"cached image queue\"? It could use a similar logic to the Verify clearing worked method.\nsuch as `self.assertIsInstance(cache_output, list)`","commit_id":"7ad7fd9b74aad1f6fbcbc4a9b964a429afbd4f0d"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"062e5a17b90c1ba5790c179dc5c7ef049a5b3ce1","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        self.addCleanup(self.openstack, \u0027image delete \u0027 + image_id)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        # Queue image for caching"},{"line_number":39,"context_line":"        self.openstack(\u0027cached image queue \u0027 + image_id)"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # Clear cached images"},{"line_number":42,"context_line":"        self.openstack(\u0027cached image clear\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"03a17306_2fd3e6ac","line":39,"in_reply_to":"55aeabda_d90aebc1","updated":"2025-09-12 09:37:39.000000000","message":"I’ve uploaded a new patchset that adds queue verification after cached image queue: asserting the parsed output type (self.assertIsInstance(cache_output, list)) and performing a brief poll of the queued list, mirroring the “Verify clearing worked” logic. I also standardized list handling for parsed output. Please review when convenient.","commit_id":"7ad7fd9b74aad1f6fbcbc4a9b964a429afbd4f0d"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"2e00b27274586664253b788db44ae50cd7962b50","unresolved":true,"context_lines":[{"line_number":36,"context_line":"        self.addCleanup(self.openstack, \u0027image delete \u0027 + image_id)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        # Queue image for caching"},{"line_number":39,"context_line":"        self.openstack(\u0027cached image queue \u0027 + image_id)"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"        # Clear cached images"},{"line_number":42,"context_line":"        self.openstack(\u0027cached image clear\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"55aeabda_d90aebc1","line":39,"in_reply_to":"d9e092ce_0e85b5b2","updated":"2025-09-11 14:31:09.000000000","message":"Thanks for the suggestion. I initially left out the queue verification because the image can remain queued only briefly, so I wasn’t sure the check would add meaningful value. I’ll add a verification step as you proposed—e.g., asserting the parsed output type (self.assertIsInstance(cache_output, list)) and/or a short poll of the queued list similar to the “Verify clearing worked” logic—and update the patch.","commit_id":"7ad7fd9b74aad1f6fbcbc4a9b964a429afbd4f0d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"e6be03ca5c195a766c8d7469294263e03a1ec81e","unresolved":true,"context_lines":[{"line_number":41,"context_line":"        # Verify queuing worked"},{"line_number":42,"context_line":"        cache_output \u003d self.openstack(\u0027cached image list\u0027, parse_output\u003dTrue)"},{"line_number":43,"context_line":"        self.assertIsInstance(cache_output, list)"},{"line_number":44,"context_line":"        if cache_output:"},{"line_number":45,"context_line":"            image_ids \u003d [img[\u0027ID\u0027] for img in cache_output]"},{"line_number":46,"context_line":"            self.assertIn(image_id, image_ids)"},{"line_number":47,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"d4ff188c_c4eaf570","line":44,"updated":"2025-09-25 10:18:16.000000000","message":"Is this conditional necessary? If so, can you add a comment explaining why? From PS5 I suspect it\u0027s a timing thing. If so, it would be good to confirm that.","commit_id":"3b30acc8c678ccbb9258ba9ea232e73ffb369d60"},{"author":{"_account_id":38257,"name":"Wonjun Choi","display_name":"wonjundero","email":"wonjundero@gmail.com","username":"wonjundero"},"change_message_id":"7cba3a038bb75e82a59011651fc4229d2bd1ca98","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        # Verify queuing worked"},{"line_number":42,"context_line":"        cache_output \u003d self.openstack(\u0027cached image list\u0027, parse_output\u003dTrue)"},{"line_number":43,"context_line":"        self.assertIsInstance(cache_output, list)"},{"line_number":44,"context_line":"        if cache_output:"},{"line_number":45,"context_line":"            image_ids \u003d [img[\u0027ID\u0027] for img in cache_output]"},{"line_number":46,"context_line":"            self.assertIn(image_id, image_ids)"},{"line_number":47,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"2464322c_18d69d15","line":44,"in_reply_to":"d4ff188c_c4eaf570","updated":"2025-09-25 14:54:09.000000000","message":"Thanks for the feedback.\n\nI revisited the conditional as you suggested.\n\nL44 – removed: After adding the image to the cached image queue, the image should be visible from cached image list, so the conditional was unnecessary. I removed it.\n\nL53 – kept: After cached image clear, the purpose is to verify that the specific image we cached was cleared; the cache does not have to be empty. Other cached images may legitimately exist due to parallel tests or the system.\nTherefore, checking the list first and then asserting that our image_id is not present is appropriate here.\n\nPlease let me know if this approach matches the intent. I’m happy to adjust based on your guidance.","commit_id":"3b30acc8c678ccbb9258ba9ea232e73ffb369d60"}]}