)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4f3c833e252e4d819b2f3c342b08e2e850721c54","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"91cab4b5_ebccbb6c","updated":"2021-11-02 01:54:57.000000000","message":"Follow up patch test coverage fails with the typo in PS1 and passes with the fixed PS2, LGTM","commit_id":"60b977b76dc113183043840acf98b215441e186d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ff7defee13a861517fc3e437150099cfcedc0c0f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"e7781f93_b1399b9e","updated":"2021-11-02 17:42:18.000000000","message":"recheck","commit_id":"60b977b76dc113183043840acf98b215441e186d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c60f7a1029732f687b45c0778737384931acb594","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"f95b1774_a5369b7f","updated":"2021-10-19 09:07:06.000000000","message":"recheck","commit_id":"60b977b76dc113183043840acf98b215441e186d"},{"author":{"_account_id":7634,"name":"Takashi Natsume","email":"takanattie@gmail.com","username":"natsumet"},"change_message_id":"fea32b4244346db9355492e098fb82373ca88f76","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"97dd904d_38691231","updated":"2021-11-02 14:04:23.000000000","message":"recheck timeout","commit_id":"60b977b76dc113183043840acf98b215441e186d"}],"nova/db/api/migrations/env.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"8e18877b9ad943d3c5b3491095b046625a199011","unresolved":true,"context_lines":[{"line_number":59,"context_line":"            (\u0027build_requests\u0027, \u0027ramdisk_id\u0027),"},{"line_number":60,"context_line":"            (\u0027build_requests\u0027, \u0027root_device_name\u0027),"},{"line_number":61,"context_line":"            (\u0027build_requests\u0027, \u0027user_data\u0027),"},{"line_number":62,"context_line":"            (\u0027resource_provides\u0027, \u0027can_host\u0027),"},{"line_number":63,"context_line":"        }"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"2999286b_58cb7f80","line":62,"range":{"start_line":62,"start_character":14,"end_line":62,"end_character":31},"updated":"2021-10-12 21:08:34.000000000","message":"resource_providers\n\nAlso, what is the significance of this list of columns? I looked at the documentation [1] but unfortunately I still don\u0027t know what it means to \"include a name\" and why some columns should not include a name.\n\nIs there a way to test this, to run this code such that we would have caught the typo? Should this have failed while running migrations?\n\n[1] https://alembic.sqlalchemy.org/en/latest/api/runtime.html#alembic.runtime.environment.EnvironmentContext.configure.params.include_name","commit_id":"51f6fa761f05edb25659792249300e79f4eda210"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"da0218f654a4c5573dbe156e5c241987ca3b9329","unresolved":true,"context_lines":[{"line_number":59,"context_line":"            (\u0027build_requests\u0027, \u0027ramdisk_id\u0027),"},{"line_number":60,"context_line":"            (\u0027build_requests\u0027, \u0027root_device_name\u0027),"},{"line_number":61,"context_line":"            (\u0027build_requests\u0027, \u0027user_data\u0027),"},{"line_number":62,"context_line":"            (\u0027resource_provides\u0027, \u0027can_host\u0027),"},{"line_number":63,"context_line":"        }"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"54411309_68a34d1a","line":62,"range":{"start_line":62,"start_character":14,"end_line":62,"end_character":31},"in_reply_to":"2999286b_58cb7f80","updated":"2021-10-13 16:25:57.000000000","message":"\u003e resource_providers\n\nOh, weird. I\u0027m not sure how this is passing...\n\n\u003e Also, what is the significance of this list of columns? I looked at the documentation [1] but unfortunately I still don\u0027t know what it means to \"include a name\" and why some columns should not include a name.\n\nThese are the docs you\u0027re looking for:\n\nhttps://alembic.sqlalchemy.org/en/latest/autogenerate.html#controlling-what-to-be-autogenerated\n\ntl;dr: the auto-generation stuff works by comparing the schema of an existing table which was created by alembic using the existing migrations, to the schema it expects for the models. The \u0027include_name\u0027 function is used to determine whether to consider an object (i.e. a given table, column, schema, etc.) in this comparison. We use this function to exclude the shadow tables when looking at the main table. These particular entries all correspond to columns that have been removed from the models but which we don\u0027t want to remove from the underlying database yet. I\u0027ll add a comment to communicate this better.\n \n\u003e Is there a way to test this, to run this code such that we would have caught the typo? Should this have failed while running migrations?\n\u003e \n\u003e [1] https://alembic.sqlalchemy.org/en/latest/api/runtime.html#alembic.runtime.environment.EnvironmentContext.configure.params.include_name\n\nTo generate this list, I simply attempted to auto-generate a migration as described in the docs [1]. You can do this yourself locally, if you want:\n\n  $ tox -e venv -- alembic -c nova/db/api/alembic.ini upgrade head\n  $ tox -e venv -- alembic -c nova/db/api/alembic.ini revision --autogenerate\n\nAnything that popped up was included in this list, once I\u0027d verified it wasn\u0027t a genuine mistake. To test this, we could write a test harness that calls the machinery that auto-generate is using under the hood to detect the same changes. There shouldn\u0027t be: if there are, our migrations are out of data. I\u0027m not sure how to do that right now so I\u0027ll have to investigate.\n\nLater: aaand after running the commands I gave above, I know the answer to my question up top: it wasn\u0027t working:\n\n  $ tox -e venv -- alembic -c nova/db/api/alembic.ini revision --autogenerate\n  ...\n  INFO  [alembic.runtime.migration] Context impl SQLiteImpl.\n  INFO  [alembic.runtime.migration] Will assume non-transactional DDL.\n  INFO  [alembic.autogenerate.compare] Detected removed column \u0027resource_providers.can_host\u0027\n    Generating /home/stephenfin/Development/openstack/nova/nova/db/api/migrations/versions/4852bec199d8_.py ...  done\n\nfat-fingers--\n\n[1] https://docs.openstack.org/nova/latest/reference/database-migrations.html#schema-migrations","commit_id":"51f6fa761f05edb25659792249300e79f4eda210"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"d9f6d180ddbf087e564357b4dfa2e53b16c6856b","unresolved":false,"context_lines":[{"line_number":59,"context_line":"            (\u0027build_requests\u0027, \u0027ramdisk_id\u0027),"},{"line_number":60,"context_line":"            (\u0027build_requests\u0027, \u0027root_device_name\u0027),"},{"line_number":61,"context_line":"            (\u0027build_requests\u0027, \u0027user_data\u0027),"},{"line_number":62,"context_line":"            (\u0027resource_provides\u0027, \u0027can_host\u0027),"},{"line_number":63,"context_line":"        }"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"be55d2b0_6a63fb3e","line":62,"range":{"start_line":62,"start_character":14,"end_line":62,"end_character":31},"in_reply_to":"54411309_68a34d1a","updated":"2021-10-13 16:47:07.000000000","message":"Turns out we already do this. The oslo.db-based tests we\u0027ve provided in [1] and [2] validate that migrations match models, and they have their own version of this \"whitelist\" of columns and tables that should be ignore. It seems we simply need to place this in a common place to avoid potential fat-fingering. I\u0027ll do that in a follow-up.\n\nPS: I grokked this by reading zzzeek\u0027s comments at [3]\n\n[1] nova/tests/unit/db/main/test_migrations.py\n[2] nova/tests/unit/db/api/test_migrations.py\n[3] https://github.com/sqlalchemy/alembic/issues/724#issuecomment-672081357","commit_id":"51f6fa761f05edb25659792249300e79f4eda210"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9e46ebf88cf8f2786e56ed7c04f42b64c9837708","unresolved":false,"context_lines":[{"line_number":59,"context_line":"            (\u0027build_requests\u0027, \u0027ramdisk_id\u0027),"},{"line_number":60,"context_line":"            (\u0027build_requests\u0027, \u0027root_device_name\u0027),"},{"line_number":61,"context_line":"            (\u0027build_requests\u0027, \u0027user_data\u0027),"},{"line_number":62,"context_line":"            (\u0027resource_provides\u0027, \u0027can_host\u0027),"},{"line_number":63,"context_line":"        }"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"d2c90d92_d3d12fce","line":62,"range":{"start_line":62,"start_character":14,"end_line":62,"end_character":31},"in_reply_to":"be55d2b0_6a63fb3e","updated":"2021-10-18 19:28:59.000000000","message":"I\u0027ve added the follow-up to move this to the common place [1]\n\n[1] https://review.opendev.org/c/openstack/nova/+/814489/1","commit_id":"51f6fa761f05edb25659792249300e79f4eda210"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4f3c833e252e4d819b2f3c342b08e2e850721c54","unresolved":true,"context_lines":[{"line_number":42,"context_line":"        # old code could attempt to access data in the removed columns. Alembic"},{"line_number":43,"context_line":"        # identifies this temporary mismatch between the models and underlying"},{"line_number":44,"context_line":"        # tables and attempts to resolve it. Tell it instead to ignore these"},{"line_number":45,"context_line":"        # until we\u0027re ready to remove them ourselves."},{"line_number":46,"context_line":"        return (parent_names[\u0027table_name\u0027], name) not in {"},{"line_number":47,"context_line":"            (\u0027build_requests\u0027, \u0027request_spec_id\u0027),"},{"line_number":48,"context_line":"            (\u0027build_requests\u0027, \u0027user_id\u0027),"}],"source_content_type":"text/x-python","patch_set":2,"id":"f6622d19_673e4b26","line":45,"updated":"2021-11-02 01:54:57.000000000","message":"++ thanks for the detailed code comment","commit_id":"60b977b76dc113183043840acf98b215441e186d"}]}