)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":35264,"name":"Alex Welsh","email":"alex@stackhpc.com","username":"alex-welsh"},"change_message_id":"9b9dbd9de4bba8d83f6bdd379b0322b7a57db9b7","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"911f31b7_ae68da13","updated":"2023-02-16 09:49:30.000000000","message":"Could you add a release note?","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":22629,"name":"Michal Nasiadka","email":"mnasiadka@gmail.com","username":"mnasiadka"},"change_message_id":"0dd21f43b38d97338678d3c51ce15a6c1fe3ebc8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"26e65b02_293211e8","updated":"2023-02-16 09:43:15.000000000","message":"Looks ok, but please add tests for this case in tests/test_set_config.py.","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":35511,"name":"Dawud","email":"dawud@stackhpc.com","username":"dawudm"},"change_message_id":"d881075cf0d2c4673b701872dd16eeb859e88bc8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"91347339_b685d584","in_reply_to":"26e65b02_293211e8","updated":"2023-02-22 18:10:27.000000000","message":"Tests have been added","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":35511,"name":"Dawud","email":"dawud@stackhpc.com","username":"dawudm"},"change_message_id":"d881075cf0d2c4673b701872dd16eeb859e88bc8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b7afbaad_88c4dba1","in_reply_to":"911f31b7_ae68da13","updated":"2023-02-22 18:10:27.000000000","message":"Added :)","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":35511,"name":"Dawud","email":"dawud@stackhpc.com","username":"dawudm"},"change_message_id":"d881075cf0d2c4673b701872dd16eeb859e88bc8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"4143aa50_6b25f47a","updated":"2023-02-22 18:10:27.000000000","message":"Added tests and removed an if statement that would not be reached due to the parent function erroring first. Also added a release note and fixed a typo.","commit_id":"b4ff9f05d90e01cfc559169bf87b3c291fd24d3e"},{"author":{"_account_id":22629,"name":"Michal Nasiadka","email":"mnasiadka@gmail.com","username":"mnasiadka"},"change_message_id":"31fdf0a5f60aa43472237325899afc8cb1f8bfb9","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":7,"id":"5995d30c_e41bd57c","updated":"2023-05-18 05:41:07.000000000","message":"Are the comments resolved? Is it ready for reviewing again?","commit_id":"d9a6c5f3901e078a30d341397ac57d867d8191cc"},{"author":{"_account_id":35511,"name":"Dawud","email":"dawud@stackhpc.com","username":"dawudm"},"change_message_id":"3f5935df53e718779baeb5b604ae80b9a6b386ff","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"0d3ffbbd_3f8d5a9f","updated":"2023-08-21 12:57:26.000000000","message":"Comments should be resolved as long as @MarkGoddard is happy with the changes and explanation provided","commit_id":"d9a6c5f3901e078a30d341397ac57d867d8191cc"},{"author":{"_account_id":35511,"name":"Dawud","email":"dawud@stackhpc.com","username":"dawudm"},"change_message_id":"6886693dc251b56124bc168a9c0afb45b44a6edc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"0fc94eb8_5f2f8a4f","updated":"2023-03-17 18:16:13.000000000","message":"recheck","commit_id":"d9a6c5f3901e078a30d341397ac57d867d8191cc"},{"author":{"_account_id":35511,"name":"Dawud","email":"dawud@stackhpc.com","username":"dawudm"},"change_message_id":"3f5935df53e718779baeb5b604ae80b9a6b386ff","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"35f8d5e0_a4472261","in_reply_to":"5995d30c_e41bd57c","updated":"2023-08-21 12:57:26.000000000","message":"Done","commit_id":"d9a6c5f3901e078a30d341397ac57d867d8191cc"}],"docker/base/set_configs.py":[{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"60629d3e24d36b196b6c4ff114a87d52a2189b96","unresolved":true,"context_lines":[{"line_number":164,"context_line":"        if (os.path.exists(source) and"},{"line_number":165,"context_line":"                not os.path.exists(dest)):"},{"line_number":166,"context_line":"            return False"},{"line_number":167,"context_line":"        if (os.path.exists(dest) and"},{"line_number":168,"context_line":"                not os.path.exists(source)):"},{"line_number":169,"context_line":"            return False"},{"line_number":170,"context_line":"        if (self.optional and"},{"line_number":171,"context_line":"                not os.path.exists(source) and"},{"line_number":172,"context_line":"                not os.path.exists(dest)):"}],"source_content_type":"text/x-python","patch_set":1,"id":"79b1c369_c8c115d2","line":169,"range":{"start_line":167,"start_character":0,"end_line":169,"end_character":24},"updated":"2023-02-16 10:14:17.000000000","message":"I think this could result in a restart loop, since AFAIK files aren\u0027t removed from the destination. I might be wrong","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":28048,"name":"Will Szumski","email":"will@stackhpc.com","username":"jovial"},"change_message_id":"ff416a79c105780c0ce37596e7e7c618187d55b1","unresolved":true,"context_lines":[{"line_number":164,"context_line":"        if (os.path.exists(source) and"},{"line_number":165,"context_line":"                not os.path.exists(dest)):"},{"line_number":166,"context_line":"            return False"},{"line_number":167,"context_line":"        if (os.path.exists(dest) and"},{"line_number":168,"context_line":"                not os.path.exists(source)):"},{"line_number":169,"context_line":"            return False"},{"line_number":170,"context_line":"        if (self.optional and"},{"line_number":171,"context_line":"                not os.path.exists(source) and"},{"line_number":172,"context_line":"                not os.path.exists(dest)):"}],"source_content_type":"text/x-python","patch_set":1,"id":"c2f06af1_67673206","line":169,"range":{"start_line":167,"start_character":0,"end_line":169,"end_character":24},"in_reply_to":"79b1c369_c8c115d2","updated":"2023-02-20 11:39:24.000000000","message":"Note at the moment we can never reach this branch due to an earlier check:\n\nhttps://github.com/openstack/kolla/blob/ca0238598a37c4a174c7b8d55b64c15bb34ef58a/docker/base/set_configs.py#L221-L222\n\nSo, potentially, we could remove this conditional block. I do think it would be nice if we could remove stale files. What do you think about making it so they did? Could we:\n\n- Remove dest if source is missing and dest is a file\n- Remove dest if source is missing, dest is a directory and the merge option (https://docs.openstack.org/kolla/ussuri/admin/kolla_api.html#format-of-the-configuration-file) is false.\n\nThe one scenario I can think of where this wouldn\u0027t work well is if you wanted to use a file that already existed in the container by default, but have the potential to override it. That seems a little bit of an anti-pattern as once you\u0027ve overridden it you can\u0027t easily restore the old contents. This could cause issues if you wanted to revert back to using the old version of the file by removing your override on the host. I Don\u0027t know if this is a pattern of usage used anywhere.\n\nI don\u0027t think returning True here would result in a restart loop as this is the code path used in `kolla_set_configs --check` as used by the compare_container action of the kolla_docker module in kolla-ansible i.e it is used to trigger a restart handler. So it would probably just result in the container being restarted every time you did a deploy (assuming we don\u0027t add logic to remove stale files). Some other code path is used on container start up (I think).","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":35511,"name":"Dawud","email":"dawud@stackhpc.com","username":"dawudm"},"change_message_id":"3f5935df53e718779baeb5b604ae80b9a6b386ff","unresolved":false,"context_lines":[{"line_number":164,"context_line":"        if (os.path.exists(source) and"},{"line_number":165,"context_line":"                not os.path.exists(dest)):"},{"line_number":166,"context_line":"            return False"},{"line_number":167,"context_line":"        if (os.path.exists(dest) and"},{"line_number":168,"context_line":"                not os.path.exists(source)):"},{"line_number":169,"context_line":"            return False"},{"line_number":170,"context_line":"        if (self.optional and"},{"line_number":171,"context_line":"                not os.path.exists(source) and"},{"line_number":172,"context_line":"                not os.path.exists(dest)):"}],"source_content_type":"text/x-python","patch_set":1,"id":"e3d9d494_095a319e","line":169,"range":{"start_line":167,"start_character":0,"end_line":169,"end_character":24},"in_reply_to":"c2f06af1_67673206","updated":"2023-08-21 12:57:26.000000000","message":"Done","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":14826,"name":"Mark Goddard","email":"markgoddard86@gmail.com","username":"mgoddard"},"change_message_id":"60629d3e24d36b196b6c4ff114a87d52a2189b96","unresolved":true,"context_lines":[{"line_number":172,"context_line":"                not os.path.exists(dest)):"},{"line_number":173,"context_line":"            return True"},{"line_number":174,"context_line":"        # check content"},{"line_number":175,"context_line":"        with open(source, \u0027rb\u0027) as f1, open(dest, \u0027rb\u0027) as f2:"},{"line_number":176,"context_line":"            if f1.read() !\u003d f2.read():"},{"line_number":177,"context_line":"                LOG.error(\u0027The content of source file(%s) and\u0027"},{"line_number":178,"context_line":"                          \u0027 dest file(%s) are not equal.\u0027, source, dest)"}],"source_content_type":"text/x-python","patch_set":1,"id":"f39e2353_0c40c305","line":175,"updated":"2023-02-16 10:14:17.000000000","message":"Could we get here if optional is false and neither file exists?","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":35511,"name":"Dawud","email":"dawud@stackhpc.com","username":"dawudm"},"change_message_id":"3f5935df53e718779baeb5b604ae80b9a6b386ff","unresolved":false,"context_lines":[{"line_number":172,"context_line":"                not os.path.exists(dest)):"},{"line_number":173,"context_line":"            return True"},{"line_number":174,"context_line":"        # check content"},{"line_number":175,"context_line":"        with open(source, \u0027rb\u0027) as f1, open(dest, \u0027rb\u0027) as f2:"},{"line_number":176,"context_line":"            if f1.read() !\u003d f2.read():"},{"line_number":177,"context_line":"                LOG.error(\u0027The content of source file(%s) and\u0027"},{"line_number":178,"context_line":"                          \u0027 dest file(%s) are not equal.\u0027, source, dest)"}],"source_content_type":"text/x-python","patch_set":1,"id":"c923eac8_5091d218","line":175,"in_reply_to":"108ca24a_f0185b75","updated":"2023-08-21 12:57:26.000000000","message":"Done","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":28048,"name":"Will Szumski","email":"will@stackhpc.com","username":"jovial"},"change_message_id":"0aa4833082bc7766f69f7900be438961715041c4","unresolved":false,"context_lines":[{"line_number":172,"context_line":"                not os.path.exists(dest)):"},{"line_number":173,"context_line":"            return True"},{"line_number":174,"context_line":"        # check content"},{"line_number":175,"context_line":"        with open(source, \u0027rb\u0027) as f1, open(dest, \u0027rb\u0027) as f2:"},{"line_number":176,"context_line":"            if f1.read() !\u003d f2.read():"},{"line_number":177,"context_line":"                LOG.error(\u0027The content of source file(%s) and\u0027"},{"line_number":178,"context_line":"                          \u0027 dest file(%s) are not equal.\u0027, source, dest)"}],"source_content_type":"text/x-python","patch_set":1,"id":"68ad8ddb_23d43bf2","line":175,"in_reply_to":"c923eac8_5091d218","updated":"2023-08-21 13:04:16.000000000","message":"Think this is covered by this test: https://review.opendev.org/c/openstack/kolla/+/873972/7/tests/test_set_config.py#349","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"},{"author":{"_account_id":28048,"name":"Will Szumski","email":"will@stackhpc.com","username":"jovial"},"change_message_id":"ff416a79c105780c0ce37596e7e7c618187d55b1","unresolved":true,"context_lines":[{"line_number":172,"context_line":"                not os.path.exists(dest)):"},{"line_number":173,"context_line":"            return True"},{"line_number":174,"context_line":"        # check content"},{"line_number":175,"context_line":"        with open(source, \u0027rb\u0027) as f1, open(dest, \u0027rb\u0027) as f2:"},{"line_number":176,"context_line":"            if f1.read() !\u003d f2.read():"},{"line_number":177,"context_line":"                LOG.error(\u0027The content of source file(%s) and\u0027"},{"line_number":178,"context_line":"                          \u0027 dest file(%s) are not equal.\u0027, source, dest)"}],"source_content_type":"text/x-python","patch_set":1,"id":"108ca24a_f0185b75","line":175,"in_reply_to":"f39e2353_0c40c305","updated":"2023-02-20 11:39:24.000000000","message":"I believe there is a check before this code:\n\nhttps://github.com/openstack/kolla/blob/ca0238598a37c4a174c7b8d55b64c15bb34ef58a/docker/base/set_configs.py#L219-L220\n\nGuess that is something to check with the testing.","commit_id":"f68b68c6d5e58831f2512dcab4792a2c9f122cf3"}]}