)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"ddaa2e26b650e21f84285f121bd6d36a84a42c6c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"7e9d3b43_69e56a9c","updated":"2025-11-18 12:10:06.000000000","message":"It is a better solution as it reuses fixtures from oslo.db, it also handles cleanups/teardowns better than watcher code. All the context_manager and engine configurations are now managed by oslo.db fixture.\nLGTM, thanks Sean.","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"808dae6105bf4f0d43562bfaf83c56b80de4fb99","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"bcf6ff98_324852f4","updated":"2025-11-18 21:20:38.000000000","message":"W+1 since there is no objection","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"4f46ac408200e0617c9209f96c73b5ffa18f640f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"2c813946_b12f43bd","updated":"2025-11-18 11:53:45.000000000","message":"thanks for the impromevent, much better to use oslo.db when possible rather than our own implementation","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"}],"watcher/tests/db/base.py":[{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"4f46ac408200e0617c9209f96c73b5ffa18f640f","unresolved":true,"context_lines":[{"line_number":31,"context_line":"CONF.import_opt(\u0027enable_authentication\u0027, \u0027watcher.api.acl\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"class SqliteDatabaseFixture(test_fixtures.GeneratesSchema,"},{"line_number":35,"context_line":"                            test_fixtures.AdHocDbFixture):"},{"line_number":36,"context_line":"    \"\"\"oslo_db-based fixture for SQLite-backed tests."},{"line_number":37,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"8240141d_84d1aa1f","line":34,"updated":"2025-11-18 11:53:45.000000000","message":"links to the fixture docs for reference https://docs.openstack.org/oslo.db/latest/reference/api/oslo_db.sqlalchemy.html#oslo_db.sqlalchemy.test_fixtures.GeneratesSchema and https://docs.openstack.org/oslo.db/latest/reference/api/oslo_db.sqlalchemy.html#oslo_db.sqlalchemy.test_fixtures.GeneratesSchema","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"ddaa2e26b650e21f84285f121bd6d36a84a42c6c","unresolved":true,"context_lines":[{"line_number":31,"context_line":"CONF.import_opt(\u0027enable_authentication\u0027, \u0027watcher.api.acl\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"class SqliteDatabaseFixture(test_fixtures.GeneratesSchema,"},{"line_number":35,"context_line":"                            test_fixtures.AdHocDbFixture):"},{"line_number":36,"context_line":"    \"\"\"oslo_db-based fixture for SQLite-backed tests."},{"line_number":37,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"50de59dd_97506302","line":34,"range":{"start_line":34,"start_character":42,"end_line":34,"end_character":57},"updated":"2025-11-18 12:10:06.000000000","message":"we could use GeneratesSchemaFromMigrations instead here, but the only difference that I see is that it will call generate_schema_migrations() instead[1]. Since we don\u0027t have a use for schema without migrations, I think that is fine too.\n\n[1] https://github.com/openstack/oslo.db/blob/5797bbddcd66ed319e0a8a5f5962eeed4e14e2ba/oslo_db/sqlalchemy/test_fixtures.py#L134-L142","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"131520d49baca5ba94294878cdd9535655b3fa3d","unresolved":false,"context_lines":[{"line_number":31,"context_line":"CONF.import_opt(\u0027enable_authentication\u0027, \u0027watcher.api.acl\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"class SqliteDatabaseFixture(test_fixtures.GeneratesSchema,"},{"line_number":35,"context_line":"                            test_fixtures.AdHocDbFixture):"},{"line_number":36,"context_line":"    \"\"\"oslo_db-based fixture for SQLite-backed tests."},{"line_number":37,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"f41f26f3_896e1fad","line":34,"range":{"start_line":34,"start_character":42,"end_line":34,"end_character":57},"in_reply_to":"50de59dd_97506302","updated":"2025-11-19 09:56:51.000000000","message":"Acknowledged","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"131520d49baca5ba94294878cdd9535655b3fa3d","unresolved":false,"context_lines":[{"line_number":31,"context_line":"CONF.import_opt(\u0027enable_authentication\u0027, \u0027watcher.api.acl\u0027)"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"class SqliteDatabaseFixture(test_fixtures.GeneratesSchema,"},{"line_number":35,"context_line":"                            test_fixtures.AdHocDbFixture):"},{"line_number":36,"context_line":"    \"\"\"oslo_db-based fixture for SQLite-backed tests."},{"line_number":37,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"8d9f9c9b_b5ddd615","line":34,"in_reply_to":"8240141d_84d1aa1f","updated":"2025-11-19 09:56:51.000000000","message":"Acknowledged","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"ddaa2e26b650e21f84285f121bd6d36a84a42c6c","unresolved":true,"context_lines":[{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"class SqliteDatabaseFixture(test_fixtures.GeneratesSchema,"},{"line_number":35,"context_line":"                            test_fixtures.AdHocDbFixture):"},{"line_number":36,"context_line":"    \"\"\"oslo_db-based fixture for SQLite-backed tests."},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    This uses oslo_db\u0027s AdHocDbFixture to provision a per-test (or per-run)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f4dd3a9_7804923f","line":35,"range":{"start_line":35,"start_character":42,"end_line":35,"end_character":56},"updated":"2025-11-18 12:10:06.000000000","message":"Ack, so the AdHoc fixture will create and dispose the db engine per test, as before.","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"131520d49baca5ba94294878cdd9535655b3fa3d","unresolved":false,"context_lines":[{"line_number":32,"context_line":""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"class SqliteDatabaseFixture(test_fixtures.GeneratesSchema,"},{"line_number":35,"context_line":"                            test_fixtures.AdHocDbFixture):"},{"line_number":36,"context_line":"    \"\"\"oslo_db-based fixture for SQLite-backed tests."},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    This uses oslo_db\u0027s AdHocDbFixture to provision a per-test (or per-run)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9fbd9ee8_7800175a","line":35,"range":{"start_line":35,"start_character":42,"end_line":35,"end_character":56},"in_reply_to":"9f4dd3a9_7804923f","updated":"2025-11-19 09:56:51.000000000","message":"Acknowledged","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4271b691f54c65cf5ae5b06be4d2558bf7973c1a","unresolved":false,"context_lines":[{"line_number":69,"context_line":"        self._id_gen \u003d utils.id_generator()"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"class MySQLDbTestCase(test_fixtures.OpportunisticDBTestMixin, base.TestCase):"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    FIXTURE \u003d test_fixtures.MySQLOpportunisticFixture"},{"line_number":75,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"6e7215df_9849dad2","line":72,"in_reply_to":"4d8ce9c4_d3fa2c36","updated":"2025-11-17 17:18:45.000000000","message":"\u003e MySQLDbTestCase inconsistent with SQLite fixture migration\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.8\n\u003e \n\u003e **Impact**: Inconsistent database setup between SQLite and MySQL test cases\n\u003e \n\u003e **Suggestion**:\n\u003e Consider updating MySQLDbTestCase to also use oslo.db\u0027s standard fixtures or document why it remains different\n\nbecause it already is :)","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4271b691f54c65cf5ae5b06be4d2558bf7973c1a","unresolved":false,"context_lines":[{"line_number":79,"context_line":"        cfg.CONF.set_override(\"connection\", conn_str,"},{"line_number":80,"context_line":"                              group\u003d\"database\")"},{"line_number":81,"context_line":"        super().setUp()"},{"line_number":82,"context_line":"        self.engine \u003d enginefacade.writer.get_engine()"},{"line_number":83,"context_line":"        self.dbapi \u003d dbapi.get_instance()"},{"line_number":84,"context_line":"        migration.create_schema()"}],"source_content_type":"text/x-python","patch_set":2,"id":"eb3687ba_9a2bac6b","line":82,"in_reply_to":"00183453_e8e10364","updated":"2025-11-17 17:18:45.000000000","message":"\u003e Optimize engine disposal in MySQLDbTestCase\n\u003e \n\u003e **Severity**: SUGGESTION | **Confidence**: 0.7\n\u003e \n\u003e **Benefit**: Better resource management and consistency with SQLite fixture patterns\n\u003e \n\u003e **Recommendation**:\n\u003e Add proper cleanup for the engine using addCleanup(engine.dispose) in MySQLDbTestCase.setUp()\n\ni dont think this is needed but we may also imporve our migration testing to add\na WalkVersionsMixin to test the applcation of each microversion.\n\nhttps://github.com/openstack/placement/blob/97b346d6660f4f3dabfcd02e04de0a3fae7d3d56/placement/tests/functional/db/test_migrations.py#L287\n\nand we can review it then but i think this will be cleaned up automaticly by the supper class.","commit_id":"68bc99ab71aa724664eaf0be507df804c9496649"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f36c7a2a74a8ad7fcd11a69baf9e8099d0c3cffe","unresolved":false,"context_lines":[{"line_number":22,"context_line":""},{"line_number":23,"context_line":"from watcher.db import api as dbapi"},{"line_number":24,"context_line":"from watcher.db.sqlalchemy import migration"},{"line_number":25,"context_line":"from watcher.tests import base"},{"line_number":26,"context_line":"from watcher.tests.db import utils"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"189dc53d_c4974c62","line":25,"in_reply_to":"4958972c_a47c682e","updated":"2025-11-19 09:57:20.000000000","message":"Acknowledged","commit_id":"8a884e3d5166678b85eaf264e518ef24b30085e5"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"131520d49baca5ba94294878cdd9535655b3fa3d","unresolved":true,"context_lines":[{"line_number":22,"context_line":""},{"line_number":23,"context_line":"from watcher.db import api as dbapi"},{"line_number":24,"context_line":"from watcher.db.sqlalchemy import migration"},{"line_number":25,"context_line":"from watcher.tests import base"},{"line_number":26,"context_line":"from watcher.tests.db import utils"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"4958972c_a47c682e","line":25,"in_reply_to":"a3501c3b_2ce026d9","updated":"2025-11-19 09:56:51.000000000","message":"\u003e Global variable _DB_CACHE removed without understanding potential performance implications\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.8\n\u003e \n\u003e **Impact**: Could impact test execution performance if the cache was providing optimization\n\u003e \n\u003e **Suggestion**:\n\u003e Consider adding a comment explaining why the global cache removal is safe, or verify that oslo.db\u0027s AdHocDbFixture provides equivalent optimization\n\nnot relevent this has been factored out into the oslo fixture.","commit_id":"8a884e3d5166678b85eaf264e518ef24b30085e5"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"131520d49baca5ba94294878cdd9535655b3fa3d","unresolved":false,"context_lines":[{"line_number":41,"context_line":"    \"\"\""},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"    def __init__(self):"},{"line_number":44,"context_line":"        # Use the configured database connection URL"},{"line_number":45,"context_line":"        # (set to sqlite:// in tests)"},{"line_number":46,"context_line":"        super().__init__(url\u003dCONF.database.connection)"},{"line_number":47,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"b4265f57_6344adcf","line":44,"in_reply_to":"feff872b_c242c2b9","updated":"2025-11-19 09:56:51.000000000","message":"\u003e Line 44 has inconsistent indentation (8 spaces) compared to other method definitions\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.9\n\u003e \n\u003e **Impact**: Code style inconsistency that may trigger linting issues\n\u003e \n\u003e **Suggestion**:\n\u003e Fix indentation to match project standard (4 spaces for method definitions)\n\nthis is because its in a class...","commit_id":"8a884e3d5166678b85eaf264e518ef24b30085e5"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"131520d49baca5ba94294878cdd9535655b3fa3d","unresolved":false,"context_lines":[{"line_number":43,"context_line":"    def __init__(self):"},{"line_number":44,"context_line":"        # Use the configured database connection URL"},{"line_number":45,"context_line":"        # (set to sqlite:// in tests)"},{"line_number":46,"context_line":"        super().__init__(url\u003dCONF.database.connection)"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"    def generate_schema_create_all(self, engine):"},{"line_number":49,"context_line":"        \"\"\"Generate the database schema for tests using migrations helpers.\"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"5ebb14b0_c1e1df43","line":46,"in_reply_to":"dd28cf10_7ac8daf4","updated":"2025-11-19 09:56:51.000000000","message":"\u003e Direct database URL dependency on CONF.database.connection without validation\n\u003e \n\u003e **Severity**: WARNING | **Confidence**: 0.7\n\u003e \n\u003e **Impact**: Could cause test failures if CONF.database.connection is not properly configured\n\u003e \n\u003e **Suggestion**:\n\u003e Add validation or fallback logic for the database URL configuration\n\nnot a reals issue. it could but we set the config value below.","commit_id":"8a884e3d5166678b85eaf264e518ef24b30085e5"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"131520d49baca5ba94294878cdd9535655b3fa3d","unresolved":false,"context_lines":[{"line_number":45,"context_line":"        # (set to sqlite:// in tests)"},{"line_number":46,"context_line":"        super().__init__(url\u003dCONF.database.connection)"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"    def generate_schema_create_all(self, engine):"},{"line_number":49,"context_line":"        \"\"\"Generate the database schema for tests using migrations helpers.\"\"\""},{"line_number":50,"context_line":"        migration.create_schema(engine\u003dengine)"},{"line_number":51,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"07df3f9a_920d47ab","line":48,"in_reply_to":"5eead6e5_6a8391ff","updated":"2025-11-19 09:56:51.000000000","message":"\u003e Consider adding a migration helper method for consistency with MySQLDbTestCase\n\u003e \n\u003e **Severity**: SUGGESTION | **Confidence**: 0.7\n\u003e \n\u003e **Benefit**: Would improve consistency between SQLite and MySQL test cases and make future maintenance easier\n\u003e \n\u003e **Recommendation**:\n\u003e Extract the migration.create_schema() call into a helper method or ensure both test cases use the same pattern for schema creation\n\ni guess what this is really saying is we could call\n\ngenerate_schema_create_all on line 84 if it was a module level fucntion \n\nwhich yes we coudl but we don\u0027t actually see a benifit in doing that for one line\n\nif this was complex yes but its not","commit_id":"8a884e3d5166678b85eaf264e518ef24b30085e5"}]}