)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"0f165071b862cedb6ffd1b7bbc914e55469e3e24","unresolved":true,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There were a number of discrepancies between the models and migrations"},{"line_number":10,"context_line":"(both sqlalchemy-migrate and alembic-based - they\u0027re identical). These"},{"line_number":11,"context_line":"need to be addressed. In this patch, we resolve discrepancies between"},{"line_number":12,"context_line":"the \u0027nullable\u0027 values, where the models field indicated \u0027nullable\u003dTrue\u0027"},{"line_number":13,"context_line":"but the migration had the default \u0027nullable\u003dFalse\u0027, or vice versa. The"},{"line_number":14,"context_line":"migrations are truth so the models are updated to reflect this."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"49d86c23_3213f1b6","line":11,"range":{"start_line":11,"start_character":11,"end_line":11,"end_character":20},"updated":"2021-10-18 18:03:37.000000000","message":"nit: I may understand why, but it\u0027s good policy to actually state the reason in the commit message.","commit_id":"922b70c28c0fc1fd61b311e67270c126c4037c31"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"0f165071b862cedb6ffd1b7bbc914e55469e3e24","unresolved":true,"context_lines":[{"line_number":10,"context_line":"(both sqlalchemy-migrate and alembic-based - they\u0027re identical). These"},{"line_number":11,"context_line":"need to be addressed. In this patch, we resolve discrepancies between"},{"line_number":12,"context_line":"the \u0027nullable\u0027 values, where the models field indicated \u0027nullable\u003dTrue\u0027"},{"line_number":13,"context_line":"but the migration had the default \u0027nullable\u003dFalse\u0027, or vice versa. The"},{"line_number":14,"context_line":"migrations are truth so the models are updated to reflect this."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: Id4bbf46d55cdbdd5060436c6d81fad7927508739"},{"line_number":17,"context_line":"Signed-off-by: Stephen Finucane \u003cstephenfin@redhat.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3ada6500_a81d2a18","line":14,"range":{"start_line":13,"start_character":67,"end_line":14,"end_character":63},"updated":"2021-10-18 18:03:37.000000000","message":"nit: The way this is phrased requires understanding of how SQLA actually uses the nullable field, so the usefulness gets limited.","commit_id":"922b70c28c0fc1fd61b311e67270c126c4037c31"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"0f165071b862cedb6ffd1b7bbc914e55469e3e24","unresolved":true,"context_lines":[{"line_number":12,"context_line":"the \u0027nullable\u0027 values, where the models field indicated \u0027nullable\u003dTrue\u0027"},{"line_number":13,"context_line":"but the migration had the default \u0027nullable\u003dFalse\u0027, or vice versa. The"},{"line_number":14,"context_line":"migrations are truth so the models are updated to reflect this."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: Id4bbf46d55cdbdd5060436c6d81fad7927508739"},{"line_number":17,"context_line":"Signed-off-by: Stephen Finucane \u003cstephenfin@redhat.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"0ba390bf_82798986","line":15,"updated":"2021-10-18 18:03:37.000000000","message":"nit: In the changes you add TODO items, but don\u0027t explain why doing it is posponed and if there are conditions for it to happen.\n\nSince there are many instances the explanation could be included in the commit message.","commit_id":"922b70c28c0fc1fd61b311e67270c126c4037c31"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"0f165071b862cedb6ffd1b7bbc914e55469e3e24","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"128df6ce_1d1fc300","updated":"2021-10-18 18:03:37.000000000","message":"recheck","commit_id":"922b70c28c0fc1fd61b311e67270c126c4037c31"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"b63ba6f6877dcc5a6f9880a376f04f2ad1e50343","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"4bc8499b_8712a2cb","updated":"2022-01-24 16:06:02.000000000","message":"I think you may have missed one inconsistency (see previous comment).","commit_id":"dc574fa7b9e5f92d7f565783c2748a34926ce1bf"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f45f6eed8f717fbfa63195f502e68085f7221eb5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"8e5d3bc4_ef607b80","updated":"2022-01-24 16:05:11.000000000","message":"Thanks, Stephen.  Quick question inline, otherwise this looks fine.","commit_id":"dc574fa7b9e5f92d7f565783c2748a34926ce1bf"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"665f49d7dd60e4cfc71e4a2fc2935e652f7b31ea","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"c7920591_0aed51fb","updated":"2022-01-24 16:28:24.000000000","message":"The inconsistency is addressed by https://review.opendev.org/c/openstack/cinder/+/826123 , os I\u0027m OK with this patch.","commit_id":"dc574fa7b9e5f92d7f565783c2748a34926ce1bf"}],"cinder/db/migrations/versions/921e1a36b076_initial.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f45f6eed8f717fbfa63195f502e68085f7221eb5","unresolved":true,"context_lines":[{"line_number":697,"context_line":""},{"line_number":698,"context_line":"    op.create_table("},{"line_number":699,"context_line":"        \u0027image_volume_cache_entries\u0027,"},{"line_number":700,"context_line":"        sa.Column(\u0027image_updated_at\u0027, sa.DateTime(timezone\u003dFalse)),"},{"line_number":701,"context_line":"        sa.Column(\u0027id\u0027, sa.Integer, primary_key\u003dTrue, nullable\u003dFalse),"},{"line_number":702,"context_line":"        sa.Column(\u0027host\u0027, sa.String(255), index\u003dTrue, nullable\u003dFalse),"},{"line_number":703,"context_line":"        sa.Column(\u0027image_id\u0027, sa.String(36), index\u003dTrue, nullable\u003dFalse),"}],"source_content_type":"text/x-python","patch_set":4,"id":"c91ddac5_e1e600b1","line":700,"range":{"start_line":700,"start_character":38,"end_line":700,"end_character":65},"updated":"2022-01-24 16:05:11.000000000","message":"(For easy reference)","commit_id":"dc574fa7b9e5f92d7f565783c2748a34926ce1bf"}],"cinder/db/sqlalchemy/models.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"0f165071b862cedb6ffd1b7bbc914e55469e3e24","unresolved":false,"context_lines":[{"line_number":1004,"context_line":""},{"line_number":1005,"context_line":"    __tablename__ \u003d \u0027encryption\u0027"},{"line_number":1006,"context_line":""},{"line_number":1007,"context_line":"    # NOTE (smcginnis): nullable\u003dTrue triggers this to not set a default"},{"line_number":1008,"context_line":"    # value, but since it\u0027s a primary key the resulting schema will end up"},{"line_number":1009,"context_line":"    # still being NOT NULL. This is avoiding a case in MySQL where it will"},{"line_number":1010,"context_line":"    # otherwise set this to NOT NULL DEFAULT \u0027\u0027. May be harmless, but"}],"source_content_type":"text/x-python","patch_set":2,"id":"fccf2536_855b1391","line":1007,"range":{"start_line":1007,"start_character":12,"end_line":1007,"end_character":21},"updated":"2021-10-18 18:03:37.000000000","message":"For other reviewers: Even if Cinder no longer adds the author on notes, I think it\u0027s acceptable here so it\u0027s clear that the comment doesn\u0027t belong to the author of this patch but somebody else.  In this case it comes from the initial migrations file.","commit_id":"922b70c28c0fc1fd61b311e67270c126c4037c31"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f45f6eed8f717fbfa63195f502e68085f7221eb5","unresolved":true,"context_lines":[{"line_number":1124,"context_line":"    cluster_name \u003d sa.Column(sa.String(255), nullable\u003dTrue)"},{"line_number":1125,"context_line":"    image_id \u003d sa.Column(sa.String(36), index\u003dTrue, nullable\u003dFalse)"},{"line_number":1126,"context_line":"    # TODO(stephenfin): Add nullable\u003dFalse"},{"line_number":1127,"context_line":"    image_updated_at \u003d sa.Column(sa.DateTime)"},{"line_number":1128,"context_line":"    volume_id \u003d sa.Column(sa.String(36), nullable\u003dFalse)"},{"line_number":1129,"context_line":"    size \u003d sa.Column(sa.Integer, nullable\u003dFalse)"},{"line_number":1130,"context_line":"    last_used \u003d sa.Column("}],"source_content_type":"text/x-python","patch_set":4,"id":"98e44719_94a50083","line":1127,"range":{"start_line":1127,"start_character":33,"end_line":1127,"end_character":44},"updated":"2022-01-24 16:05:11.000000000","message":"should this be \u0027sa.DateTime(timezone\u003dFalse)\u0027 ? (Like at line 841 above; it\u0027s set explicitly in the migration file)","commit_id":"dc574fa7b9e5f92d7f565783c2748a34926ce1bf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"e7e1ec21f84727b8579c0a787384ee72d2212ea7","unresolved":false,"context_lines":[{"line_number":1124,"context_line":"    cluster_name \u003d sa.Column(sa.String(255), nullable\u003dTrue)"},{"line_number":1125,"context_line":"    image_id \u003d sa.Column(sa.String(36), index\u003dTrue, nullable\u003dFalse)"},{"line_number":1126,"context_line":"    # TODO(stephenfin): Add nullable\u003dFalse"},{"line_number":1127,"context_line":"    image_updated_at \u003d sa.Column(sa.DateTime)"},{"line_number":1128,"context_line":"    volume_id \u003d sa.Column(sa.String(36), nullable\u003dFalse)"},{"line_number":1129,"context_line":"    size \u003d sa.Column(sa.Integer, nullable\u003dFalse)"},{"line_number":1130,"context_line":"    last_used \u003d sa.Column("}],"source_content_type":"text/x-python","patch_set":4,"id":"f50bd843_4ad334fe","line":1127,"range":{"start_line":1127,"start_character":33,"end_line":1127,"end_character":44},"in_reply_to":"586a9b18_b91c2fcb","updated":"2022-01-24 16:23:01.000000000","message":"https://review.opendev.org/c/openstack/cinder/+/826123","commit_id":"dc574fa7b9e5f92d7f565783c2748a34926ce1bf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"172dca5bd2743b745fbfad5ff8e8116ca804b21a","unresolved":true,"context_lines":[{"line_number":1124,"context_line":"    cluster_name \u003d sa.Column(sa.String(255), nullable\u003dTrue)"},{"line_number":1125,"context_line":"    image_id \u003d sa.Column(sa.String(36), index\u003dTrue, nullable\u003dFalse)"},{"line_number":1126,"context_line":"    # TODO(stephenfin): Add nullable\u003dFalse"},{"line_number":1127,"context_line":"    image_updated_at \u003d sa.Column(sa.DateTime)"},{"line_number":1128,"context_line":"    volume_id \u003d sa.Column(sa.String(36), nullable\u003dFalse)"},{"line_number":1129,"context_line":"    size \u003d sa.Column(sa.Integer, nullable\u003dFalse)"},{"line_number":1130,"context_line":"    last_used \u003d sa.Column("}],"source_content_type":"text/x-python","patch_set":4,"id":"586a9b18_b91c2fcb","line":1127,"range":{"start_line":1127,"start_character":33,"end_line":1127,"end_character":44},"in_reply_to":"98e44719_94a50083","updated":"2022-01-24 16:16:12.000000000","message":"Good spot. Fortunately this isn\u0027t an issue since \u0027timezone\u003dFalse\u0027 is the default [1], but I agree that we should align the two to avoid confusion. Let me whip up a follow-up\n\n[1] https://docs.sqlalchemy.org/en/14/core/type_basics.html#sqlalchemy.types.DateTime","commit_id":"dc574fa7b9e5f92d7f565783c2748a34926ce1bf"}],"cinder/tests/unit/objects/test_objects.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"0f165071b862cedb6ffd1b7bbc914e55469e3e24","unresolved":true,"context_lines":[{"line_number":120,"context_line":"                if name \u003d\u003d \u0027Group\u0027 and column.name \u003d\u003d \u0027group_type_id\u0027:"},{"line_number":121,"context_line":"                    continue"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"                # TODO(stephenfin): Model and Object don\u0027t match here, but it"},{"line_number":124,"context_line":"                # wasn\u0027t intentional"},{"line_number":125,"context_line":"                if (name, column.name) in {"},{"line_number":126,"context_line":"                    (\u0027Backup\u0027, \u0027project_id\u0027),"},{"line_number":127,"context_line":"                    (\u0027Backup\u0027, \u0027user_id\u0027),"}],"source_content_type":"text/x-python","patch_set":2,"id":"897c13f0_ce52afa2","line":124,"range":{"start_line":123,"start_character":0,"end_line":124,"end_character":36},"updated":"2021-10-18 18:03:37.000000000","message":"nit: The text doesn\u0027t explain what the TODO is, and it relies on actually reading the commit message and the other TODOs from the patch...","commit_id":"922b70c28c0fc1fd61b311e67270c126c4037c31"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"0f165071b862cedb6ffd1b7bbc914e55469e3e24","unresolved":true,"context_lines":[{"line_number":131,"context_line":"                    (\u0027CGSnapshot\u0027, \u0027user_id\u0027),"},{"line_number":132,"context_line":"                    (\u0027ConsistencyGroup\u0027, \u0027project_id\u0027),"},{"line_number":133,"context_line":"                    (\u0027ConsistencyGroup\u0027, \u0027user_id\u0027),"},{"line_number":134,"context_line":"                    (\u0027Group\u0027, \u0027user_id\u0027),"},{"line_number":135,"context_line":"                    (\u0027Group\u0027, \u0027project_id\u0027),"},{"line_number":136,"context_line":"                    (\u0027GroupType\u0027, \u0027name\u0027),"},{"line_number":137,"context_line":"                    (\u0027Service\u0027, \u0027frozen\u0027),"},{"line_number":138,"context_line":"                    (\u0027Snapshot\u0027, \u0027volume_type_id\u0027),"}],"source_content_type":"text/x-python","patch_set":2,"id":"0c7def00_6d299e92","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":44},"updated":"2021-10-18 18:03:37.000000000","message":"nit: This one has project_id and user_id lines order changed.","commit_id":"922b70c28c0fc1fd61b311e67270c126c4037c31"}]}