)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"b2c6a4417a5efc0e5c7116712b01b952ef2f4445","unresolved":false,"context_lines":[{"line_number":1,"context_line":"Parent:     c7e29560 (Merge \"Small fixes for WebOb 1.7 compatibiltity\")"},{"line_number":2,"context_line":"Author:     Richard Avelar \u003ccsravelar@gmail.com\u003e"},{"line_number":3,"context_line":"AuthorDate: 2017-02-23 15:35:21 +0000"},{"line_number":4,"context_line":"Commit:     Ubuntu \u003ccsravelar@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-03-27 17:48:39 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Validate rolling upgrade is run in order"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"ffe62b97_b21ff059","line":4,"updated":"2017-03-27 19:48:19.000000000","message":"You should probably reconfigure git:\n\n  git config --global user.name \"Richard Avelar\"\n  git commit --amend --no-edit \u0026\u0026 git-review","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"}],"keystone/cmd/cli.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"a25e2f1db9bf164d5d796a99b7fbf78f02813702","unresolved":false,"context_lines":[{"line_number":478,"context_line":"                expand_version):"},{"line_number":479,"context_line":"            LOG.info(_LI(\u0027Your database is out of sync. For more information \u0027"},{"line_number":480,"context_line":"                         \u0027refer to https://docs.openstack.org/developer/\u0027"},{"line_number":481,"context_line":"                         \u0027keystone/upgrading.html\u0027))"},{"line_number":482,"context_line":"            status \u003d 1"},{"line_number":483,"context_line":"        elif migration_script_version \u003e expand_version:"},{"line_number":484,"context_line":"            LOG.info(_LI(\u0027Your database is not up to date. Your first step is \u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a30ddce_8d0afc1f","side":"PARENT","line":481,"updated":"2017-03-08 22:34:20.000000000","message":"I think this is the message Dolph was referring to in his review of patch set 7. I wouldn\u0027t mind keeping this message around since it is helpful and it points people to docs in the event things go haywire.","commit_id":"724c9b7cd91a97fca8af7f7ec4ec8ef3d450681b"}],"keystone/common/sql/upgrades.py":[{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"13e1bf061cb935c720c69e1edce61ae7532ac5f5","unresolved":false,"context_lines":[{"line_number":262,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":265,"context_line":"    have expand run before migrate and migrate run before expand."},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    \"\"\""},{"line_number":268,"context_line":"    db_sync_order \u003d {\u0027expand_repo\u0027:\u0027migrate_repo\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"da36d5c6_043f8c87","line":265,"range":{"start_line":265,"start_character":58,"end_line":265,"end_character":64},"updated":"2017-02-23 15:38:38.000000000","message":"contract","commit_id":"e4a1c95f5108f0e918231d7de5c9af9dc74e9195"},{"author":{"_account_id":22604,"name":"Samuel Pilla","email":"sp516w@att.com","username":"samuel.pilla"},"change_message_id":"65564a926542cc58faaa73581662ab98cb3b17b3","unresolved":false,"context_lines":[{"line_number":261,"context_line":""},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"def validate_upgrade_order(repo_name, version\u003dNone):"},{"line_number":264,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":267,"context_line":"    have expand run before migrate and migrate run before expand. Compare that"}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_12712de1","line":264,"range":{"start_line":264,"start_character":36,"end_line":264,"end_character":38},"updated":"2017-02-27 19:19:16.000000000","message":"is*","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"fae630ed9d20c10e4a36a99b64ac7856d598c428","unresolved":false,"context_lines":[{"line_number":261,"context_line":""},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"def validate_upgrade_order(repo_name, version\u003dNone):"},{"line_number":264,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":267,"context_line":"    have expand run before migrate and migrate run before expand. Compare that"}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_d7bdbfd8","line":264,"range":{"start_line":264,"start_character":36,"end_line":264,"end_character":38},"updated":"2017-02-27 19:15:49.000000000","message":"nit: is","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ab85321a32f9c37c457c04b414cd7b86d15a8383","unresolved":false,"context_lines":[{"line_number":261,"context_line":""},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"def validate_upgrade_order(repo_name, version\u003dNone):"},{"line_number":264,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":267,"context_line":"    have expand run before migrate and migrate run before expand. Compare that"}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_c6ec2a95","line":264,"range":{"start_line":264,"start_character":36,"end_line":264,"end_character":38},"in_reply_to":"da36d5c6_12712de1","updated":"2017-02-27 22:06:57.000000000","message":"Done","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ab85321a32f9c37c457c04b414cd7b86d15a8383","unresolved":false,"context_lines":[{"line_number":261,"context_line":""},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"def validate_upgrade_order(repo_name, version\u003dNone):"},{"line_number":264,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":267,"context_line":"    have expand run before migrate and migrate run before expand. Compare that"}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_a6e1f69c","line":264,"range":{"start_line":264,"start_character":36,"end_line":264,"end_character":38},"in_reply_to":"da36d5c6_d7bdbfd8","updated":"2017-02-27 22:06:57.000000000","message":"Done","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":22604,"name":"Samuel Pilla","email":"sp516w@att.com","username":"samuel.pilla"},"change_message_id":"65564a926542cc58faaa73581662ab98cb3b17b3","unresolved":false,"context_lines":[{"line_number":263,"context_line":"def validate_upgrade_order(repo_name, version\u003dNone):"},{"line_number":264,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":267,"context_line":"    have expand run before migrate and migrate run before expand. Compare that"},{"line_number":268,"context_line":"    the current command the operator wants to run will not be greater than the"},{"line_number":269,"context_line":"    previous command."},{"line_number":270,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_d5293f8b","line":267,"range":{"start_line":266,"start_character":61,"end_line":267,"end_character":65},"updated":"2017-02-27 19:19:16.000000000","message":"I\u0027d consider rewording this so it\u0027s more clear which expand, migrate, and contract sync versions need to run when.","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"fae630ed9d20c10e4a36a99b64ac7856d598c428","unresolved":false,"context_lines":[{"line_number":264,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":267,"context_line":"    have expand run before migrate and migrate run before expand. Compare that"},{"line_number":268,"context_line":"    the current command the operator wants to run will not be greater than the"},{"line_number":269,"context_line":"    previous command."},{"line_number":270,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_b7b4cbd4","line":267,"range":{"start_line":267,"start_character":58,"end_line":267,"end_character":64},"updated":"2017-02-27 19:15:49.000000000","message":"contract","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ab85321a32f9c37c457c04b414cd7b86d15a8383","unresolved":false,"context_lines":[{"line_number":264,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":267,"context_line":"    have expand run before migrate and migrate run before expand. Compare that"},{"line_number":268,"context_line":"    the current command the operator wants to run will not be greater than the"},{"line_number":269,"context_line":"    previous command."},{"line_number":270,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_86e4f2ac","line":267,"range":{"start_line":267,"start_character":58,"end_line":267,"end_character":64},"in_reply_to":"da36d5c6_b7b4cbd4","updated":"2017-02-27 22:06:57.000000000","message":"Done","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ab85321a32f9c37c457c04b414cd7b86d15a8383","unresolved":false,"context_lines":[{"line_number":263,"context_line":"def validate_upgrade_order(repo_name, version\u003dNone):"},{"line_number":264,"context_line":"    \"\"\"Validate the db_sync command it not run out of order."},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync needs to"},{"line_number":267,"context_line":"    have expand run before migrate and migrate run before expand. Compare that"},{"line_number":268,"context_line":"    the current command the operator wants to run will not be greater than the"},{"line_number":269,"context_line":"    previous command."},{"line_number":270,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_e6f12e6c","line":267,"range":{"start_line":266,"start_character":61,"end_line":267,"end_character":65},"in_reply_to":"da36d5c6_d5293f8b","updated":"2017-02-27 22:06:57.000000000","message":"Done","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"fae630ed9d20c10e4a36a99b64ac7856d598c428","unresolved":false,"context_lines":[{"line_number":280,"context_line":"    repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":281,"context_line":"    current_cmd_version \u003d version or int(max(repo.versions.versions))"},{"line_number":282,"context_line":"    # get the version of the command that is run before the current one."},{"line_number":283,"context_line":"    previous_cmd_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    if previous_cmd_version \u003c current_cmd_version:"},{"line_number":286,"context_line":"        raise Exception(\u0027Version (%d) of %s is greater than the version (%d) \u0027"}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_37245bc5","line":283,"range":{"start_line":283,"start_character":4,"end_line":283,"end_character":12},"updated":"2017-02-27 19:15:49.000000000","message":"nit: Not sure I agree with the choice of variable name here. ``current`` and ``previous`` is very ambiguous, especially when referring to upgrading (and since it\u0027s not really current but the upgrade target version). Something like ``target_repo_version`` and ``dependency_repo_version`` might be better.","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ab85321a32f9c37c457c04b414cd7b86d15a8383","unresolved":false,"context_lines":[{"line_number":280,"context_line":"    repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":281,"context_line":"    current_cmd_version \u003d version or int(max(repo.versions.versions))"},{"line_number":282,"context_line":"    # get the version of the command that is run before the current one."},{"line_number":283,"context_line":"    previous_cmd_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    if previous_cmd_version \u003c current_cmd_version:"},{"line_number":286,"context_line":"        raise Exception(\u0027Version (%d) of %s is greater than the version (%d) \u0027"}],"source_content_type":"text/x-python","patch_set":3,"id":"da36d5c6_06b5627d","line":283,"range":{"start_line":283,"start_character":4,"end_line":283,"end_character":12},"in_reply_to":"da36d5c6_37245bc5","updated":"2017-02-27 22:06:57.000000000","message":"Done","commit_id":"4d6f7709ffe537027089b073c36a4105cf94e384"},{"author":{"_account_id":22604,"name":"Samuel Pilla","email":"sp516w@att.com","username":"samuel.pilla"},"change_message_id":"80820f0bd00eb3b0356261745bcccb9d3a7533e7","unresolved":false,"context_lines":[{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync must have"},{"line_number":267,"context_line":"    expand, migrate, and contract run in that order. Compare the current"},{"line_number":268,"context_line":"    command upgrade version the operator wants to run will not be greater than"},{"line_number":269,"context_line":"    the preceding command\u0027s upgrade version."},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"ba2be162_91cfdeb2","line":268,"range":{"start_line":268,"start_character":4,"end_line":268,"end_character":11},"updated":"2017-02-28 14:26:33.000000000","message":"command\u0027s","commit_id":"5b49f9011fe9f93bbd1aedb011673b4d5b12ee03"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"2aa1efc7d56c637d638370168337671c7e554b92","unresolved":false,"context_lines":[{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    This is run before allowing the db_sync command execute. db_sync must have"},{"line_number":267,"context_line":"    expand, migrate, and contract run in that order. Compare the current"},{"line_number":268,"context_line":"    command upgrade version the operator wants to run will not be greater than"},{"line_number":269,"context_line":"    the preceding command\u0027s upgrade version."},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"ba2be162_534a9584","line":268,"range":{"start_line":268,"start_character":4,"end_line":268,"end_character":11},"in_reply_to":"ba2be162_91cfdeb2","updated":"2017-03-01 15:24:34.000000000","message":"Done","commit_id":"5b49f9011fe9f93bbd1aedb011673b4d5b12ee03"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"9e8564020c25aa56de19d0542f440c9399a8b70b","unresolved":false,"context_lines":[{"line_number":276,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":277,"context_line":"        return"},{"line_number":278,"context_line":"    # find the version that the current command will upgrade to."},{"line_number":279,"context_line":"    abs_path \u003d find_repo(repo_name)"},{"line_number":280,"context_line":"    repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":281,"context_line":"    target_repo_version \u003d version or int(max(repo.versions.versions))"},{"line_number":282,"context_line":"    # get the version of the command that is run before the current one."},{"line_number":283,"context_line":"    dependency_repo_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba2be162_b84c5343","line":280,"range":{"start_line":279,"start_character":0,"end_line":280,"end_character":61},"updated":"2017-03-02 14:05:12.000000000","message":"why not only do this if \"version\" is None?","commit_id":"3c206e1d68500a66e1534aaef7c0e315febb98c4"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"e1d5de81fabf4e784330bb89b3dc8adf7ac34c7b","unresolved":false,"context_lines":[{"line_number":276,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":277,"context_line":"        return"},{"line_number":278,"context_line":"    # find the version that the current command will upgrade to."},{"line_number":279,"context_line":"    abs_path \u003d find_repo(repo_name)"},{"line_number":280,"context_line":"    repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":281,"context_line":"    target_repo_version \u003d version or int(max(repo.versions.versions))"},{"line_number":282,"context_line":"    # get the version of the command that is run before the current one."},{"line_number":283,"context_line":"    dependency_repo_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba2be162_99030e42","line":280,"range":{"start_line":279,"start_character":0,"end_line":280,"end_character":61},"in_reply_to":"ba2be162_b84c5343","updated":"2017-03-02 16:07:17.000000000","message":"Done","commit_id":"3c206e1d68500a66e1534aaef7c0e315febb98c4"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":261,"context_line":"            session.get_bind(), find_repo(repo), get_init_version())"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"def validate_upgrade_order(repo_name, target_repo_version\u003dNone):"},{"line_number":265,"context_line":"    \"\"\"Validate the db_sync command is not run out of order."},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command execute. db_sync must have"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_4031b004","line":264,"updated":"2017-03-08 20:37:49.000000000","message":"Why only validate the state of one repo, instead of just validating them all, like the old code intended to do?","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":261,"context_line":"            session.get_bind(), find_repo(repo), get_init_version())"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"def validate_upgrade_order(repo_name, target_repo_version\u003dNone):"},{"line_number":265,"context_line":"    \"\"\"Validate the db_sync command is not run out of order."},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command execute. db_sync must have"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_347ebdae","line":264,"in_reply_to":"9a30ddce_4031b004","updated":"2017-03-08 21:56:40.000000000","message":"Elaborate? Why unnecessarily validate everything else every time when you are only running a single command at a time which is dependent on the db_sync command that must be run before it?","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":268,"context_line":"    expand, migrate, and contract run in that order. Compare the current"},{"line_number":269,"context_line":"    command\u0027s upgrade version the operator wants to run will not be greater"},{"line_number":270,"context_line":"    than the preceding command\u0027s upgrade version."},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    \"\"\""},{"line_number":273,"context_line":"    db_sync_order \u003d {\u0027expand_repo\u0027: \u0027migrate_repo\u0027,"},{"line_number":274,"context_line":"                     \u0027data_migration_repo\u0027: \u0027expand_repo\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_d469e1bf","line":271,"updated":"2017-03-08 20:37:49.000000000","message":"Documenting what the parameters actually mean would be helpful to understand what this method is actually validating.","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":268,"context_line":"    expand, migrate, and contract run in that order. Compare the current"},{"line_number":269,"context_line":"    command\u0027s upgrade version the operator wants to run will not be greater"},{"line_number":270,"context_line":"    than the preceding command\u0027s upgrade version."},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    \"\"\""},{"line_number":273,"context_line":"    db_sync_order \u003d {\u0027expand_repo\u0027: \u0027migrate_repo\u0027,"},{"line_number":274,"context_line":"                     \u0027data_migration_repo\u0027: \u0027expand_repo\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_17361be6","line":271,"in_reply_to":"9a30ddce_d469e1bf","updated":"2017-03-08 21:56:40.000000000","message":"++ Done","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":272,"context_line":"    \"\"\""},{"line_number":273,"context_line":"    db_sync_order \u003d {\u0027expand_repo\u0027: \u0027migrate_repo\u0027,"},{"line_number":274,"context_line":"                     \u0027data_migration_repo\u0027: \u0027expand_repo\u0027,"},{"line_number":275,"context_line":"                     \u0027contract_repo\u0027: \u0027data_migration_repo\u0027}"},{"line_number":276,"context_line":"    # validate the repo_name is one of the db_sync commands or exit."},{"line_number":277,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":278,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_e08704d5","line":275,"updated":"2017-03-08 20:37:49.000000000","message":"This dictionary is super confusing, and deserves an inline comment to explain what the keys and values actually mean. Also, use the constants at the top of the module.","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":272,"context_line":"    \"\"\""},{"line_number":273,"context_line":"    db_sync_order \u003d {\u0027expand_repo\u0027: \u0027migrate_repo\u0027,"},{"line_number":274,"context_line":"                     \u0027data_migration_repo\u0027: \u0027expand_repo\u0027,"},{"line_number":275,"context_line":"                     \u0027contract_repo\u0027: \u0027data_migration_repo\u0027}"},{"line_number":276,"context_line":"    # validate the repo_name is one of the db_sync commands or exit."},{"line_number":277,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":278,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_b7862f11","line":275,"in_reply_to":"9a30ddce_e08704d5","updated":"2017-03-08 21:56:40.000000000","message":"Done","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":273,"context_line":"    db_sync_order \u003d {\u0027expand_repo\u0027: \u0027migrate_repo\u0027,"},{"line_number":274,"context_line":"                     \u0027data_migration_repo\u0027: \u0027expand_repo\u0027,"},{"line_number":275,"context_line":"                     \u0027contract_repo\u0027: \u0027data_migration_repo\u0027}"},{"line_number":276,"context_line":"    # validate the repo_name is one of the db_sync commands or exit."},{"line_number":277,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":278,"context_line":"        return"},{"line_number":279,"context_line":"    # find the version that the current command will upgrade to."}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_80648822","line":276,"updated":"2017-03-08 20:37:49.000000000","message":"This comment just re-iterates what the code is doing, not why it\u0027s doing it. Why is it safe to exit early? Why would repo_name ever not be in the set of known keys?","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":273,"context_line":"    db_sync_order \u003d {\u0027expand_repo\u0027: \u0027migrate_repo\u0027,"},{"line_number":274,"context_line":"                     \u0027data_migration_repo\u0027: \u0027expand_repo\u0027,"},{"line_number":275,"context_line":"                     \u0027contract_repo\u0027: \u0027data_migration_repo\u0027}"},{"line_number":276,"context_line":"    # validate the repo_name is one of the db_sync commands or exit."},{"line_number":277,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":278,"context_line":"        return"},{"line_number":279,"context_line":"    # find the version that the current command will upgrade to."}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_977dab70","line":276,"in_reply_to":"9a30ddce_80648822","updated":"2017-03-08 21:56:40.000000000","message":"Legacy Repository. https://github.com/openstack/keystone/blob/master/keystone/tests/unit/test_sql_upgrade.py#L226-L228","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"77a721a97221283f84c2bf3df3fbc81d2178c05f","unresolved":false,"context_lines":[{"line_number":273,"context_line":"    db_sync_order \u003d {\u0027expand_repo\u0027: \u0027migrate_repo\u0027,"},{"line_number":274,"context_line":"                     \u0027data_migration_repo\u0027: \u0027expand_repo\u0027,"},{"line_number":275,"context_line":"                     \u0027contract_repo\u0027: \u0027data_migration_repo\u0027}"},{"line_number":276,"context_line":"    # validate the repo_name is one of the db_sync commands or exit."},{"line_number":277,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":278,"context_line":"        return"},{"line_number":279,"context_line":"    # find the version that the current command will upgrade to."}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_cb43a06a","line":276,"in_reply_to":"9a30ddce_977dab70","updated":"2017-03-10 19:32:38.000000000","message":"Those were not questions for code review, they were questions that your inline comment should be preemptively answering.","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":276,"context_line":"    # validate the repo_name is one of the db_sync commands or exit."},{"line_number":277,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":278,"context_line":"        return"},{"line_number":279,"context_line":"    # find the version that the current command will upgrade to."},{"line_number":280,"context_line":"    if not target_repo_version:"},{"line_number":281,"context_line":"        abs_path \u003d find_repo(repo_name)"},{"line_number":282,"context_line":"        repo \u003d migrate.versioning.repository.Repository(abs_path)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_c045a070","line":279,"updated":"2017-03-08 20:37:49.000000000","message":"Why is this argument optional?","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":276,"context_line":"    # validate the repo_name is one of the db_sync commands or exit."},{"line_number":277,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":278,"context_line":"        return"},{"line_number":279,"context_line":"    # find the version that the current command will upgrade to."},{"line_number":280,"context_line":"    if not target_repo_version:"},{"line_number":281,"context_line":"        abs_path \u003d find_repo(repo_name)"},{"line_number":282,"context_line":"        repo \u003d migrate.versioning.repository.Repository(abs_path)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_97f4cbf2","line":279,"in_reply_to":"9a30ddce_c045a070","updated":"2017-03-08 21:56:40.000000000","message":"Because db_sync command doesn\u0027t need a version to be supplied by the user and can upgrade to the latest when none is given?","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":281,"context_line":"        abs_path \u003d find_repo(repo_name)"},{"line_number":282,"context_line":"        repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":283,"context_line":"        target_repo_version \u003d int(max(repo.versions.versions))"},{"line_number":284,"context_line":"    # get the version of the command that is run before the current one."},{"line_number":285,"context_line":"    dependency_repo_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    if dependency_repo_version \u003c target_repo_version:"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_f436e5e5","line":284,"updated":"2017-03-08 20:37:49.000000000","message":"?","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":281,"context_line":"        abs_path \u003d find_repo(repo_name)"},{"line_number":282,"context_line":"        repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":283,"context_line":"        target_repo_version \u003d int(max(repo.versions.versions))"},{"line_number":284,"context_line":"    # get the version of the command that is run before the current one."},{"line_number":285,"context_line":"    dependency_repo_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    if dependency_repo_version \u003c target_repo_version:"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_374b1fb2","line":284,"in_reply_to":"9a30ddce_f436e5e5","updated":"2017-03-08 21:56:40.000000000","message":"As outlined here: https://bugs.launchpad.net/keystone/+bug/1615014\nWe validate the command that the user wants to run is not attempting to upgrade ahead of the command that comes before it. Hope this helps","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    if dependency_repo_version \u003c target_repo_version:"},{"line_number":288,"context_line":"        raise db_exception.DBMigrationError("},{"line_number":289,"context_line":"            \u0027Version (%d) of %s is greater than the version (%d) of %s.\u0027 % ("},{"line_number":290,"context_line":"                target_repo_version, repo_name,"},{"line_number":291,"context_line":"                dependency_repo_version, db_sync_order[repo_name]))"},{"line_number":292,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_c0592000","line":289,"updated":"2017-03-08 20:37:49.000000000","message":"The operator-focused error message removed in this patch describes roughly what is wrong and provides additional information as to what recourse the operator has (the goal being to help operators understand, and hopefully resolve, the situation).\n\nBy contrast, this error message is just a mathematical statement that doesn\u0027t even imply that something is wrong about the condition being described. The error might as well say \"1 + 1 \u003d 2\",","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    if dependency_repo_version \u003c target_repo_version:"},{"line_number":288,"context_line":"        raise db_exception.DBMigrationError("},{"line_number":289,"context_line":"            \u0027Version (%d) of %s is greater than the version (%d) of %s.\u0027 % ("},{"line_number":290,"context_line":"                target_repo_version, repo_name,"},{"line_number":291,"context_line":"                dependency_repo_version, db_sync_order[repo_name]))"},{"line_number":292,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_57b7b3d7","line":289,"in_reply_to":"9a30ddce_c0592000","updated":"2017-03-08 21:56:40.000000000","message":"\"removed in this patch\"? What is being removed?","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"a25e2f1db9bf164d5d796a99b7fbf78f02813702","unresolved":false,"context_lines":[{"line_number":296,"context_line":"    if dependency_repo_version \u003c target_repo_version:"},{"line_number":297,"context_line":"        raise db_exception.DBMigrationError("},{"line_number":298,"context_line":"            \u0027Version (%d) of %s is greater than the version (%d) of %s. %s \u0027"},{"line_number":299,"context_line":"            \u0027version must be upgraded higher than or equivalent to %s \u0027"},{"line_number":300,"context_line":"            \u0027version.\u0027 % (target_repo_version, repo_name,"},{"line_number":301,"context_line":"                          dependency_repo_version, db_sync_order[repo_name],"},{"line_number":302,"context_line":"                          db_sync_order[repo_name], repo_name))"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a30ddce_2d69108c","line":299,"updated":"2017-03-08 22:34:20.000000000","message":"This could be generalized like the error in cli.py, as an operator that typically gives me a little more to go on since it\u0027s including documentation.","commit_id":"352d5ed62d6b3b110b57092ec8b1eb5e328fa102"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"4ed739889a2032d7757502af10fef815bacb1922","unresolved":false,"context_lines":[{"line_number":296,"context_line":"    if dependency_repo_version \u003c target_repo_version:"},{"line_number":297,"context_line":"        raise db_exception.DBMigrationError("},{"line_number":298,"context_line":"            \u0027Version (%d) of %s is greater than the version (%d) of %s. %s \u0027"},{"line_number":299,"context_line":"            \u0027version must be upgraded higher than or equivalent to %s \u0027"},{"line_number":300,"context_line":"            \u0027version.\u0027 % (target_repo_version, repo_name,"},{"line_number":301,"context_line":"                          dependency_repo_version, db_sync_order[repo_name],"},{"line_number":302,"context_line":"                          db_sync_order[repo_name], repo_name))"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a30ddce_a3a25549","line":299,"in_reply_to":"9a30ddce_2d69108c","updated":"2017-03-09 15:43:50.000000000","message":"Done","commit_id":"352d5ed62d6b3b110b57092ec8b1eb5e328fa102"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"02c54a7ebd0c091b4a9eb20a22b4ccf176bf6862","unresolved":false,"context_lines":[{"line_number":262,"context_line":""},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"def validate_upgrade_order(repo_name, target_repo_version\u003dNone):"},{"line_number":265,"context_line":"    \"\"\"Validate the db_sync command is not run out of order."},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command execute. db_sync must have"},{"line_number":268,"context_line":"    expand, migrate, and contract run in that order. Compare the current"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_fe42e0ac","line":265,"updated":"2017-03-10 19:49:59.000000000","message":"This module doesn\u0027t know anything about \"the db_sync command\" from keystone-manage, as it\u0027s a lower-level module concerned with sqlalchemy migrate\u0027s db_sync *function*. Rather, you\u0027re validating the state of the migration repos and the intended change of state.","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"8c79f4a007319e95f32999ce9008385cd7de09f5","unresolved":false,"context_lines":[{"line_number":262,"context_line":""},{"line_number":263,"context_line":""},{"line_number":264,"context_line":"def validate_upgrade_order(repo_name, target_repo_version\u003dNone):"},{"line_number":265,"context_line":"    \"\"\"Validate the db_sync command is not run out of order."},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command execute. db_sync must have"},{"line_number":268,"context_line":"    expand, migrate, and contract run in that order. Compare the current"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_1ccedc62","line":265,"in_reply_to":"9a30ddce_fe42e0ac","updated":"2017-03-10 21:28:45.000000000","message":"Done","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"02c54a7ebd0c091b4a9eb20a22b4ccf176bf6862","unresolved":false,"context_lines":[{"line_number":270,"context_line":"    than the preceding command\u0027s upgrade version."},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":273,"context_line":"                      ugrade."},{"line_number":274,"context_line":"    :param target_repo_version: The version number that user is trying to"},{"line_number":275,"context_line":"                                upgrade the repository too. If None is"},{"line_number":276,"context_line":"                                specified for this parameter then this will be"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_7e6190a8","line":273,"updated":"2017-03-10 19:49:59.000000000","message":"spelling. in vi:\n\n  set spell spelllang\u003den_us","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"8c79f4a007319e95f32999ce9008385cd7de09f5","unresolved":false,"context_lines":[{"line_number":270,"context_line":"    than the preceding command\u0027s upgrade version."},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":273,"context_line":"                      ugrade."},{"line_number":274,"context_line":"    :param target_repo_version: The version number that user is trying to"},{"line_number":275,"context_line":"                                upgrade the repository too. If None is"},{"line_number":276,"context_line":"                                specified for this parameter then this will be"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_bc31505c","line":273,"in_reply_to":"9a30ddce_7e6190a8","updated":"2017-03-10 21:28:45.000000000","message":"Done","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"02c54a7ebd0c091b4a9eb20a22b4ccf176bf6862","unresolved":false,"context_lines":[{"line_number":272,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":273,"context_line":"                      ugrade."},{"line_number":274,"context_line":"    :param target_repo_version: The version number that user is trying to"},{"line_number":275,"context_line":"                                upgrade the repository too. If None is"},{"line_number":276,"context_line":"                                specified for this parameter then this will be"},{"line_number":277,"context_line":"                                upgraded to the latest version available."},{"line_number":278,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_7ebd10d7","line":275,"updated":"2017-03-10 19:49:59.000000000","message":"\"None\" (likely) won\u0027t be specified explicitly, since it\u0027s the default parameter.\n\ns/too/to/","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"8c79f4a007319e95f32999ce9008385cd7de09f5","unresolved":false,"context_lines":[{"line_number":272,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":273,"context_line":"                      ugrade."},{"line_number":274,"context_line":"    :param target_repo_version: The version number that user is trying to"},{"line_number":275,"context_line":"                                upgrade the repository too. If None is"},{"line_number":276,"context_line":"                                specified for this parameter then this will be"},{"line_number":277,"context_line":"                                upgraded to the latest version available."},{"line_number":278,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_fc8588ce","line":275,"in_reply_to":"9a30ddce_7ebd10d7","updated":"2017-03-10 21:28:45.000000000","message":"Done","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"02c54a7ebd0c091b4a9eb20a22b4ccf176bf6862","unresolved":false,"context_lines":[{"line_number":277,"context_line":"                                upgraded to the latest version available."},{"line_number":278,"context_line":"    \"\"\""},{"line_number":279,"context_line":"    # Initialize a dict to have each db_sync command as a key and their parent"},{"line_number":280,"context_line":"    # command as their value."},{"line_number":281,"context_line":"    db_sync_order \u003d {EXPAND_REPO: LEGACY_REPO,"},{"line_number":282,"context_line":"                     DATA_MIGRATION_REPO: EXPAND_REPO,"},{"line_number":283,"context_line":"                     CONTRACT_REPO: DATA_MIGRATION_REPO}"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_1e941441","line":280,"updated":"2017-03-10 19:49:59.000000000","message":"You\u0027re making up the term \u0027parent command\u0027 here, which isn\u0027t helpful. Explain the relationship between the keys and values and how the dictionary is to be used.","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"e167b5c0951cdeb6e46086707e9958c6ae49e6ee","unresolved":false,"context_lines":[{"line_number":289,"context_line":"    if not target_repo_version:"},{"line_number":290,"context_line":"        abs_path \u003d find_repo(repo_name)"},{"line_number":291,"context_line":"        repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":292,"context_line":"        target_repo_version \u003d int(max(repo.versions.versions))"},{"line_number":293,"context_line":"    # get current version of the command that runs before the current command."},{"line_number":294,"context_line":"    dependency_repo_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"},{"line_number":295,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_3f49dfb7","line":292,"updated":"2017-03-10 16:10:23.000000000","message":"Why not use repo.latest? \nhttps://sqlalchemy-migrate.readthedocs.io/en/latest/api.html#migrate.versioning.repository.Repository.latest","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"02c54a7ebd0c091b4a9eb20a22b4ccf176bf6862","unresolved":false,"context_lines":[{"line_number":289,"context_line":"    if not target_repo_version:"},{"line_number":290,"context_line":"        abs_path \u003d find_repo(repo_name)"},{"line_number":291,"context_line":"        repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":292,"context_line":"        target_repo_version \u003d int(max(repo.versions.versions))"},{"line_number":293,"context_line":"    # get current version of the command that runs before the current command."},{"line_number":294,"context_line":"    dependency_repo_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"},{"line_number":295,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_7e3a709a","line":292,"in_reply_to":"9a30ddce_3f49dfb7","updated":"2017-03-10 19:49:59.000000000","message":"I\u0027m not sure I knew that was a thing, but +1","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"8c79f4a007319e95f32999ce9008385cd7de09f5","unresolved":false,"context_lines":[{"line_number":289,"context_line":"    if not target_repo_version:"},{"line_number":290,"context_line":"        abs_path \u003d find_repo(repo_name)"},{"line_number":291,"context_line":"        repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":292,"context_line":"        target_repo_version \u003d int(max(repo.versions.versions))"},{"line_number":293,"context_line":"    # get current version of the command that runs before the current command."},{"line_number":294,"context_line":"    dependency_repo_version \u003d get_db_version(repo\u003ddb_sync_order[repo_name])"},{"line_number":295,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_7718d375","line":292,"in_reply_to":"9a30ddce_3f49dfb7","updated":"2017-03-10 21:28:45.000000000","message":"Nice! Thanks for the link","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"e167b5c0951cdeb6e46086707e9958c6ae49e6ee","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            \u0027You are attempting to upgrade %s ahead of %s. Please refer to \u0027"},{"line_number":299,"context_line":"            \u0027https://docs.openstack.org/developer/keystone/upgrading.html \u0027"},{"line_number":300,"context_line":"            \u0027to see the proper steps for rolling upgrades.\u0027 % ("},{"line_number":301,"context_line":"                repo_name, db_sync_order[repo_name]))"},{"line_number":302,"context_line":""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"def expand_schema():"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_f37f85dd","line":301,"updated":"2017-03-10 16:10:23.000000000","message":"This causes a nasty traceback to be emitted to the screen, I wonder if it might be more friendly to catch the except in keystone/cmd/cli.py and just log the message and return an error code?","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"41a42bb7361caad1d948d408f127057edcee8a93","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            \u0027You are attempting to upgrade %s ahead of %s. Please refer to \u0027"},{"line_number":299,"context_line":"            \u0027https://docs.openstack.org/developer/keystone/upgrading.html \u0027"},{"line_number":300,"context_line":"            \u0027to see the proper steps for rolling upgrades.\u0027 % ("},{"line_number":301,"context_line":"                repo_name, db_sync_order[repo_name]))"},{"line_number":302,"context_line":""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"def expand_schema():"}],"source_content_type":"text/x-python","patch_set":10,"id":"ffe62b97_b29c900e","line":301,"in_reply_to":"9a30ddce_0ad3d760","updated":"2017-03-27 19:50:33.000000000","message":"+2","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"02c54a7ebd0c091b4a9eb20a22b4ccf176bf6862","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            \u0027You are attempting to upgrade %s ahead of %s. Please refer to \u0027"},{"line_number":299,"context_line":"            \u0027https://docs.openstack.org/developer/keystone/upgrading.html \u0027"},{"line_number":300,"context_line":"            \u0027to see the proper steps for rolling upgrades.\u0027 % ("},{"line_number":301,"context_line":"                repo_name, db_sync_order[repo_name]))"},{"line_number":302,"context_line":""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"def expand_schema():"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_9e956434","line":301,"in_reply_to":"9a30ddce_14123a57","updated":"2017-03-10 19:49:59.000000000","message":"++ I do think it\u0027s appropriate to raise an exception here (as opposed to printing anything, for example), because this module does not know, and should not presume to know, what it\u0027s caller is.","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"11769bf84e4ccebb6f46f085f41ad3dbd31f47e5","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            \u0027You are attempting to upgrade %s ahead of %s. Please refer to \u0027"},{"line_number":299,"context_line":"            \u0027https://docs.openstack.org/developer/keystone/upgrading.html \u0027"},{"line_number":300,"context_line":"            \u0027to see the proper steps for rolling upgrades.\u0027 % ("},{"line_number":301,"context_line":"                repo_name, db_sync_order[repo_name]))"},{"line_number":302,"context_line":""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"def expand_schema():"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_0ad3d760","line":301,"in_reply_to":"9a30ddce_97e0e7ce","updated":"2017-03-13 08:13:31.000000000","message":"When I said \"catch the except[tion] in keystone/cmd/cli.py\" I wasn\u0027t advocating for removing the exception here, I just meant it would be nice to catch it in keystone-manage.","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"8c79f4a007319e95f32999ce9008385cd7de09f5","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            \u0027You are attempting to upgrade %s ahead of %s. Please refer to \u0027"},{"line_number":299,"context_line":"            \u0027https://docs.openstack.org/developer/keystone/upgrading.html \u0027"},{"line_number":300,"context_line":"            \u0027to see the proper steps for rolling upgrades.\u0027 % ("},{"line_number":301,"context_line":"                repo_name, db_sync_order[repo_name]))"},{"line_number":302,"context_line":""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"def expand_schema():"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_97e0e7ce","line":301,"in_reply_to":"9a30ddce_9e956434","updated":"2017-03-10 21:28:45.000000000","message":"I have to agree with dolph. And the convention for lower level modules (as can be seen throughout this file) seems to be raising the corresponding exception.","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"4c9afce2fc654612d27ec20420020954dfc72422","unresolved":false,"context_lines":[{"line_number":298,"context_line":"            \u0027You are attempting to upgrade %s ahead of %s. Please refer to \u0027"},{"line_number":299,"context_line":"            \u0027https://docs.openstack.org/developer/keystone/upgrading.html \u0027"},{"line_number":300,"context_line":"            \u0027to see the proper steps for rolling upgrades.\u0027 % ("},{"line_number":301,"context_line":"                repo_name, db_sync_order[repo_name]))"},{"line_number":302,"context_line":""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"def expand_schema():"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_14123a57","line":301,"in_reply_to":"9a30ddce_f37f85dd","updated":"2017-03-10 18:08:01.000000000","message":"++","commit_id":"70d39e1ae3adf2d769b7fe55b0390b7b5603bd02"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"50ec347b6be4971754ca35be374915f9d8817b4e","unresolved":false,"context_lines":[{"line_number":264,"context_line":"def validate_upgrade_order(repo_name, target_repo_version\u003dNone):"},{"line_number":265,"context_line":"    \"\"\"Validate the state of the migration repositories."},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command execute. db_sync must have"},{"line_number":268,"context_line":"    expand, migrate, and contract run in that order. Compare the current"},{"line_number":269,"context_line":"    command\u0027s upgrade version the operator wants to run will not be greater"},{"line_number":270,"context_line":"    than the preceding command\u0027s upgrade version."}],"source_content_type":"text/x-python","patch_set":14,"id":"9a30ddce_36e9d8b7","line":267,"updated":"2017-03-13 14:47:33.000000000","message":"s/command/command to/","commit_id":"430ae2a067f03196673aadea8af9dc4c4cc09fe4"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"50ec347b6be4971754ca35be374915f9d8817b4e","unresolved":false,"context_lines":[{"line_number":267,"context_line":"    This is run before allowing the db_sync command execute. db_sync must have"},{"line_number":268,"context_line":"    expand, migrate, and contract run in that order. Compare the current"},{"line_number":269,"context_line":"    command\u0027s upgrade version the operator wants to run will not be greater"},{"line_number":270,"context_line":"    than the preceding command\u0027s upgrade version."},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":273,"context_line":"                      upgrade."}],"source_content_type":"text/x-python","patch_set":14,"id":"9a30ddce_81f080ca","line":270,"updated":"2017-03-13 14:47:33.000000000","message":"The last two sentences are difficult to read. I\u0027d suggest playing with the wording so that both read more naturally. Maybe something like the following?\n\n\"Ensure the upgrade step and version specified by the operator remains consistent with the upgrade process\"\n\nSometimes it is easier to describe complicated situations by using examples.","commit_id":"430ae2a067f03196673aadea8af9dc4c4cc09fe4"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"50ec347b6be4971754ca35be374915f9d8817b4e","unresolved":false,"context_lines":[{"line_number":279,"context_line":"    # Initialize a dict to have each key assigned a repo with their value being"},{"line_number":280,"context_line":"    # the repo that comes before. The expand repo key therefore has a value of"},{"line_number":281,"context_line":"    # the legacy repo, migration repo key has a value of the expand repo, and"},{"line_number":282,"context_line":"    # so on. repo_name passed in will match a key and thus give us the"},{"line_number":283,"context_line":"    # corresponding repo we would like to validate against."},{"line_number":284,"context_line":"    db_sync_order \u003d {EXPAND_REPO: LEGACY_REPO,"},{"line_number":285,"context_line":"                     DATA_MIGRATION_REPO: EXPAND_REPO,"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a30ddce_41984878","line":282,"updated":"2017-03-13 14:47:33.000000000","message":"nit s/repo_name/The repo_name/","commit_id":"430ae2a067f03196673aadea8af9dc4c4cc09fe4"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"1d07c239d3a4cb163ca0dade6e771dcbc54e7050","unresolved":false,"context_lines":[{"line_number":259,"context_line":"            session.get_bind(), find_repo(repo), get_init_version())"},{"line_number":260,"context_line":""},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"def expand_schema():"},{"line_number":263,"context_line":"    \"\"\"Expand the database schema ahead of data migration."},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    This is run manually by the keystone-manage command before the first"}],"source_content_type":"text/x-python","patch_set":15,"id":"ffe62b97_92043a19","side":"PARENT","line":262,"updated":"2017-03-24 19:05:37.000000000","message":"According a coverage report generated from this patch, this is untested.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"807e77fc165ec69d778db50ea2cbd8777e2f06e5","unresolved":false,"context_lines":[{"line_number":259,"context_line":"            session.get_bind(), find_repo(repo), get_init_version())"},{"line_number":260,"context_line":""},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"def expand_schema():"},{"line_number":263,"context_line":"    \"\"\"Expand the database schema ahead of data migration."},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    This is run manually by the keystone-manage command before the first"}],"source_content_type":"text/x-python","patch_set":15,"id":"ffe62b97_6060bca6","side":"PARENT","line":262,"in_reply_to":"ffe62b97_92043a19","updated":"2017-03-24 19:50:26.000000000","message":"Actually - it turns out that this is a byproduct of the tests using a different entry point to this functionality than that cli does. The cli uses these methods to expand, migrate, and contract schemas. The unit tests within keystone use upgrade and pass the path/version of the repository to upgrade.\n\nIn the future, maybe we can look into consolidating this so that there is a single entry point for this. As a result, we won\u0027t have false positives in our coverage document and the overall design will be easier to understand from a developer perspective.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"1d07c239d3a4cb163ca0dade6e771dcbc54e7050","unresolved":false,"context_lines":[{"line_number":272,"context_line":"    _sync_repo(repo_name\u003dEXPAND_REPO)"},{"line_number":273,"context_line":""},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"def migrate_data():"},{"line_number":276,"context_line":"    \"\"\"Migrate data to match the new schema."},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"    This is run manually by the keystone-manage command once the keystone"}],"source_content_type":"text/x-python","patch_set":15,"id":"ffe62b97_150ed433","side":"PARENT","line":275,"updated":"2017-03-24 19:05:37.000000000","message":"Same comment here.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"807e77fc165ec69d778db50ea2cbd8777e2f06e5","unresolved":false,"context_lines":[{"line_number":272,"context_line":"    _sync_repo(repo_name\u003dEXPAND_REPO)"},{"line_number":273,"context_line":""},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"def migrate_data():"},{"line_number":276,"context_line":"    \"\"\"Migrate data to match the new schema."},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"    This is run manually by the keystone-manage command once the keystone"}],"source_content_type":"text/x-python","patch_set":15,"id":"ffe62b97_60bc3cdc","side":"PARENT","line":275,"in_reply_to":"ffe62b97_150ed433","updated":"2017-03-24 19:50:26.000000000","message":"Same comment as above.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"1d07c239d3a4cb163ca0dade6e771dcbc54e7050","unresolved":false,"context_lines":[{"line_number":282,"context_line":"    _sync_repo(repo_name\u003dDATA_MIGRATION_REPO)"},{"line_number":283,"context_line":""},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"def contract_schema():"},{"line_number":286,"context_line":"    \"\"\"Contract the database."},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    This is run manually by the keystone-manage command once the keystone"}],"source_content_type":"text/x-python","patch_set":15,"id":"ffe62b97_d50f4c2c","side":"PARENT","line":285,"updated":"2017-03-24 19:05:37.000000000","message":"Same comment here.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"807e77fc165ec69d778db50ea2cbd8777e2f06e5","unresolved":false,"context_lines":[{"line_number":282,"context_line":"    _sync_repo(repo_name\u003dDATA_MIGRATION_REPO)"},{"line_number":283,"context_line":""},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"def contract_schema():"},{"line_number":286,"context_line":"    \"\"\"Contract the database."},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"    This is run manually by the keystone-manage command once the keystone"}],"source_content_type":"text/x-python","patch_set":15,"id":"ffe62b97_40c13854","side":"PARENT","line":285,"in_reply_to":"ffe62b97_d50f4c2c","updated":"2017-03-24 19:50:26.000000000","message":"Same comment as above.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"1d07c239d3a4cb163ca0dade6e771dcbc54e7050","unresolved":false,"context_lines":[{"line_number":288,"context_line":"        return"},{"line_number":289,"context_line":"    # find the latest version that the current command will upgrade to if there"},{"line_number":290,"context_line":"    # wasn\u0027t a version specified for upgrade."},{"line_number":291,"context_line":"    if not target_repo_version:"},{"line_number":292,"context_line":"        abs_path \u003d find_repo(repo_name)"},{"line_number":293,"context_line":"        repo \u003d migrate.versioning.repository.Repository(abs_path)"},{"line_number":294,"context_line":"        target_repo_version \u003d int(repo.latest)"}],"source_content_type":"text/x-python","patch_set":15,"id":"ffe62b97_552d7c93","line":291,"updated":"2017-03-24 19:05:37.000000000","message":"This case seems to be untested according to http://104.130.175.68/cover/keystone_common_sql_upgrades_py.html","commit_id":"b6de95db648dbdb421bccb82b09e3150b2c53b9c"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"971cfb64b30f3585b76af506a0a787c2d483e40e","unresolved":false,"context_lines":[{"line_number":276,"context_line":"                                available."},{"line_number":277,"context_line":"    \"\"\""},{"line_number":278,"context_line":"    # Initialize a dict to have each key assigned a repo with their value being"},{"line_number":279,"context_line":"    # the repo that comes before. The expand repo key therefore has a value of"},{"line_number":280,"context_line":"    # the legacy repo, migration repo key has a value of the expand repo, and"},{"line_number":281,"context_line":"    # so on. The repo_name passed in will match a key and thus give us the"},{"line_number":282,"context_line":"    # corresponding repo we would like to validate against."}],"source_content_type":"text/x-python","patch_set":17,"id":"ffe62b97_3ef9774c","line":279,"updated":"2017-03-27 16:38:45.000000000","message":"The expand repo key...*and so on. Should be removed. You are just repeating what the code says.","commit_id":"d33f257fcc46003768c1cf14246aef37a26e89e4"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"50eef383ad480a2bf007f2ccd8c83d13edcdaa46","unresolved":false,"context_lines":[{"line_number":276,"context_line":"                                available."},{"line_number":277,"context_line":"    \"\"\""},{"line_number":278,"context_line":"    # Initialize a dict to have each key assigned a repo with their value being"},{"line_number":279,"context_line":"    # the repo that comes before. The expand repo key therefore has a value of"},{"line_number":280,"context_line":"    # the legacy repo, migration repo key has a value of the expand repo, and"},{"line_number":281,"context_line":"    # so on. The repo_name passed in will match a key and thus give us the"},{"line_number":282,"context_line":"    # corresponding repo we would like to validate against."}],"source_content_type":"text/x-python","patch_set":17,"id":"ffe62b97_814b7f29","line":279,"in_reply_to":"ffe62b97_3ef9774c","updated":"2017-03-27 17:45:59.000000000","message":"Agreed. I initially only added this extra bit due to comments on \nhttps://review.openstack.org/#/c/437441/10..17/keystone/common/sql/upgrades.py\nBut I agree with the redundancy.","commit_id":"d33f257fcc46003768c1cf14246aef37a26e89e4"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"971cfb64b30f3585b76af506a0a787c2d483e40e","unresolved":false,"context_lines":[{"line_number":284,"context_line":"                     DATA_MIGRATION_REPO: EXPAND_REPO,"},{"line_number":285,"context_line":"                     CONTRACT_REPO: DATA_MIGRATION_REPO}"},{"line_number":286,"context_line":"    # exit if we are just trying to upgrade the Legacy repository."},{"line_number":287,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":288,"context_line":"        return"},{"line_number":289,"context_line":"    # find the latest version that the current command will upgrade to if there"},{"line_number":290,"context_line":"    # wasn\u0027t a version specified for upgrade."}],"source_content_type":"text/x-python","patch_set":17,"id":"ffe62b97_de0e0b8c","line":287,"updated":"2017-03-27 16:38:45.000000000","message":".keys() is redundant.\n\nAny reason you don\u0027t just check to see if it is the LEGACY_REPO explicitly? They you wouldn\u0027t need the comment on 286.","commit_id":"d33f257fcc46003768c1cf14246aef37a26e89e4"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"ac1eaba86260c8bbac6d60774ece7e142634e25d","unresolved":false,"context_lines":[{"line_number":284,"context_line":"                     DATA_MIGRATION_REPO: EXPAND_REPO,"},{"line_number":285,"context_line":"                     CONTRACT_REPO: DATA_MIGRATION_REPO}"},{"line_number":286,"context_line":"    # exit if we are just trying to upgrade the Legacy repository."},{"line_number":287,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":288,"context_line":"        return"},{"line_number":289,"context_line":"    # find the latest version that the current command will upgrade to if there"},{"line_number":290,"context_line":"    # wasn\u0027t a version specified for upgrade."}],"source_content_type":"text/x-python","patch_set":17,"id":"ffe62b97_412fb7a0","line":287,"in_reply_to":"ffe62b97_01bd2fd7","updated":"2017-03-27 17:51:44.000000000","message":"Iterating over a dict always yields a stream of keys. You also define the dict inline above.","commit_id":"d33f257fcc46003768c1cf14246aef37a26e89e4"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"50eef383ad480a2bf007f2ccd8c83d13edcdaa46","unresolved":false,"context_lines":[{"line_number":284,"context_line":"                     DATA_MIGRATION_REPO: EXPAND_REPO,"},{"line_number":285,"context_line":"                     CONTRACT_REPO: DATA_MIGRATION_REPO}"},{"line_number":286,"context_line":"    # exit if we are just trying to upgrade the Legacy repository."},{"line_number":287,"context_line":"    if repo_name not in db_sync_order.keys():"},{"line_number":288,"context_line":"        return"},{"line_number":289,"context_line":"    # find the latest version that the current command will upgrade to if there"},{"line_number":290,"context_line":"    # wasn\u0027t a version specified for upgrade."}],"source_content_type":"text/x-python","patch_set":17,"id":"ffe62b97_01bd2fd7","line":287,"in_reply_to":"ffe62b97_de0e0b8c","updated":"2017-03-27 17:45:59.000000000","message":"Ah, I believe I was just concerned about repo_name also being some random string (used to C way of thinking where you constantly check things and never trust anyone). Will fix","commit_id":"d33f257fcc46003768c1cf14246aef37a26e89e4"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"b2c6a4417a5efc0e5c7116712b01b952ef2f4445","unresolved":false,"context_lines":[{"line_number":269,"context_line":"    # Make sure all the legacy migrations are run before we run any new"},{"line_number":270,"context_line":"    # expand migrations."},{"line_number":271,"context_line":"    _sync_common_repo(version\u003dNone)"},{"line_number":272,"context_line":"    _sync_repo(repo_name\u003dEXPAND_REPO)"},{"line_number":273,"context_line":""},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"def migrate_data():"}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_f2b9f8cb","side":"PARENT","line":272,"updated":"2017-03-27 19:48:19.000000000","message":"Does anything check that the expand repo can be run safely? (that all legacy migrations have been run previously)","commit_id":"c7e29560b7bf7a44e44722eea0645bf18ad56af3"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"b2c6a4417a5efc0e5c7116712b01b952ef2f4445","unresolved":false,"context_lines":[{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command to execute. Ensure the"},{"line_number":268,"context_line":"    upgrade step and version specified by the operator remains consistent with"},{"line_number":269,"context_line":"    the upgrade process. I.e. legacy \u003e\u003d expand, expand \u003e\u003d migrate, etc."},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":272,"context_line":"                      upgrade."}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_3219a021","line":269,"updated":"2017-03-27 19:48:19.000000000","message":"Expecting \"legacy \u003e\u003d expand\" does not make any sense, as eventually the expand repo version number will overtake the legacy repository version number, and that\u0027s fine. The legacy repo is just frozen. The legacy repo should be at the maximum version before allowing ANY schema expansions, data migrations, or schema contractions to be run, but that\u0027s it. The version number otherwise does not matter.\n\nI also don\u0027t think you a need an \"etc\" when you might as well enumerate all the rules.","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"84205ee5f5cfcb54cbcba95a58e4964de416e58a","unresolved":false,"context_lines":[{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command to execute. Ensure the"},{"line_number":268,"context_line":"    upgrade step and version specified by the operator remains consistent with"},{"line_number":269,"context_line":"    the upgrade process. I.e. legacy \u003e\u003d expand, expand \u003e\u003d migrate, etc."},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":272,"context_line":"                      upgrade."}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_e1efe01b","line":269,"in_reply_to":"ffe62b97_3219a021","updated":"2017-03-27 21:54:17.000000000","message":"Done","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"b2c6a4417a5efc0e5c7116712b01b952ef2f4445","unresolved":false,"context_lines":[{"line_number":271,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":272,"context_line":"                      upgrade."},{"line_number":273,"context_line":"    :param target_repo_version: The version number that user is trying to"},{"line_number":274,"context_line":"                                upgrade the repository to. Otherwise, the"},{"line_number":275,"context_line":"                                version will be upgraded to the latest version"},{"line_number":276,"context_line":"                                available."},{"line_number":277,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_32c76067","line":274,"updated":"2017-03-27 19:48:19.000000000","message":"Ending a sentence with a preposition?! This is the sort of bloody nonsense up with which I will not put.","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"84205ee5f5cfcb54cbcba95a58e4964de416e58a","unresolved":false,"context_lines":[{"line_number":271,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":272,"context_line":"                      upgrade."},{"line_number":273,"context_line":"    :param target_repo_version: The version number that user is trying to"},{"line_number":274,"context_line":"                                upgrade the repository to. Otherwise, the"},{"line_number":275,"context_line":"                                version will be upgraded to the latest version"},{"line_number":276,"context_line":"                                available."},{"line_number":277,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_a1f558ef","line":274,"in_reply_to":"ffe62b97_32c76067","updated":"2017-03-27 21:54:17.000000000","message":"so petty :p","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"23d713f677c634e071fd36698b75645aba1fc6f1","unresolved":false,"context_lines":[{"line_number":269,"context_line":"    # Make sure all the legacy migrations are run before we run any new"},{"line_number":270,"context_line":"    # expand migrations."},{"line_number":271,"context_line":"    _sync_common_repo(version\u003dNone)"},{"line_number":272,"context_line":"    _sync_repo(repo_name\u003dEXPAND_REPO)"},{"line_number":273,"context_line":""},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"def migrate_data():"}],"source_content_type":"text/x-python","patch_set":20,"id":"ffe62b97_283d8bc3","side":"PARENT","line":272,"updated":"2017-03-28 14:04:02.000000000","message":"Does anything check that the expand repo can be run safely? (that all legacy migrations have been run previously)","commit_id":"c7e29560b7bf7a44e44722eea0645bf18ad56af3"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"23d713f677c634e071fd36698b75645aba1fc6f1","unresolved":false,"context_lines":[{"line_number":280,"context_line":"                     CONTRACT_REPO: DATA_MIGRATION_REPO}"},{"line_number":281,"context_line":""},{"line_number":282,"context_line":"    if repo_name \u003d\u003d LEGACY_REPO or repo_name \u003d\u003d EXPAND_REPO:"},{"line_number":283,"context_line":"        return"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    # find the latest version that the current command will upgrade to if there"},{"line_number":286,"context_line":"    # wasn\u0027t a version specified for upgrade."}],"source_content_type":"text/x-python","patch_set":20,"id":"ffe62b97_08ce4781","line":283,"updated":"2017-03-28 14:04:02.000000000","message":"If the repo you\u0027re validating is the expand repo, then you should be validating that the legacy repo has been run to completion.","commit_id":"ca9e958e155c8d2eef40ddde041ed5709e27a315"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"44b3315aafbc0bf39293f4f11cd1dfa08c109e2a","unresolved":false,"context_lines":[{"line_number":280,"context_line":"                     CONTRACT_REPO: DATA_MIGRATION_REPO}"},{"line_number":281,"context_line":""},{"line_number":282,"context_line":"    if repo_name \u003d\u003d LEGACY_REPO or repo_name \u003d\u003d EXPAND_REPO:"},{"line_number":283,"context_line":"        return"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    # find the latest version that the current command will upgrade to if there"},{"line_number":286,"context_line":"    # wasn\u0027t a version specified for upgrade."}],"source_content_type":"text/x-python","patch_set":20,"id":"ffe62b97_f84bf227","line":283,"in_reply_to":"ffe62b97_08ce4781","updated":"2017-03-28 14:51:47.000000000","message":"You just mentioned in the last comment that testing expand doesn\u0027t make any sense? In addition, we already have _sync_common_repo which is inside expand_schema to ensure legacy repo has run.","commit_id":"ca9e958e155c8d2eef40ddde041ed5709e27a315"},{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"6daca59e929946f2836ff03d49533b83d2c11b9b","unresolved":false,"context_lines":[{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command to execute. Ensure the"},{"line_number":268,"context_line":"    upgrade step and version specified by the operator remains consistent with"},{"line_number":269,"context_line":"    the upgrade process. I.e. expand \u003e\u003d migrate, migrate \u003e\u003d contract."},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":272,"context_line":"                      upgrade."}],"source_content_type":"text/x-python","patch_set":28,"id":"bff0334d_8ec65967","line":269,"range":{"start_line":269,"start_character":30,"end_line":269,"end_character":47},"updated":"2017-04-06 12:11:53.000000000","message":"expand before migrate ? these math operators got me confused","commit_id":"ea91a90a54794734ec92ac76397b7519a9f86fc1"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"114520c800d3b2571fa1291dbfcb03299f47648c","unresolved":false,"context_lines":[{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    This is run before allowing the db_sync command to execute. Ensure the"},{"line_number":268,"context_line":"    upgrade step and version specified by the operator remains consistent with"},{"line_number":269,"context_line":"    the upgrade process. I.e. expand \u003e\u003d migrate, migrate \u003e\u003d contract."},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"    :param repo_name: The name of the repository that the user is trying to"},{"line_number":272,"context_line":"                      upgrade."}],"source_content_type":"text/x-python","patch_set":28,"id":"bff0334d_0846b924","line":269,"range":{"start_line":269,"start_character":30,"end_line":269,"end_character":47},"in_reply_to":"bff0334d_8ec65967","updated":"2017-04-06 14:53:33.000000000","message":"Done","commit_id":"ea91a90a54794734ec92ac76397b7519a9f86fc1"}],"keystone/tests/unit/test_sql_upgrade.py":[{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"9e8564020c25aa56de19d0542f440c9399a8b70b","unresolved":false,"context_lines":[{"line_number":1667,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1668,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1669,"context_line":""},{"line_number":1670,"context_line":"        # Running contract before migrate to a higher version should raise an"},{"line_number":1671,"context_line":"        # exception."},{"line_number":1672,"context_line":"        try:"},{"line_number":1673,"context_line":"            self.contract(2)"},{"line_number":1674,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba2be162_d8910754","line":1671,"range":{"start_line":1670,"start_character":0,"end_line":1671,"end_character":20},"updated":"2017-03-02 14:05:12.000000000","message":"also add a test for the version number? Like: running everything up to version 2 and try to run self.expand(1) again.","commit_id":"3c206e1d68500a66e1534aaef7c0e315febb98c4"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"e1d5de81fabf4e784330bb89b3dc8adf7ac34c7b","unresolved":false,"context_lines":[{"line_number":1667,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1668,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1669,"context_line":""},{"line_number":1670,"context_line":"        # Running contract before migrate to a higher version should raise an"},{"line_number":1671,"context_line":"        # exception."},{"line_number":1672,"context_line":"        try:"},{"line_number":1673,"context_line":"            self.contract(2)"},{"line_number":1674,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba2be162_7939fa40","line":1671,"range":{"start_line":1670,"start_character":0,"end_line":1671,"end_character":20},"in_reply_to":"ba2be162_d8910754","updated":"2017-03-02 16:07:17.000000000","message":"This patch is to address running a db_sync command ahead of another to a higher version. Downgrading is already prevented and tested here: https://github.com/openstack/keystone/blob/master/keystone/tests/unit/test_sql_upgrade.py#L1601-L1606","commit_id":"3c206e1d68500a66e1534aaef7c0e315febb98c4"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"9e8564020c25aa56de19d0542f440c9399a8b70b","unresolved":false,"context_lines":[{"line_number":1669,"context_line":""},{"line_number":1670,"context_line":"        # Running contract before migrate to a higher version should raise an"},{"line_number":1671,"context_line":"        # exception."},{"line_number":1672,"context_line":"        try:"},{"line_number":1673,"context_line":"            self.contract(2)"},{"line_number":1674,"context_line":"        except Exception as e:"},{"line_number":1675,"context_line":"            self.assertIn(\u0027contract_repo is greater than\u0027, repr(e))"},{"line_number":1676,"context_line":"        else:"},{"line_number":1677,"context_line":"            self.fail(\u0027Running upgrades out of order did not raise a conflict\u0027)"},{"line_number":1678,"context_line":""},{"line_number":1679,"context_line":"    def test_db_sync_check(self):"},{"line_number":1680,"context_line":"        checker \u003d cli.DbSync()"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba2be162_d89f474e","line":1677,"range":{"start_line":1672,"start_character":0,"end_line":1677,"end_character":79},"updated":"2017-03-02 14:05:12.000000000","message":"is there a reason for not using assertRaise here?","commit_id":"3c206e1d68500a66e1534aaef7c0e315febb98c4"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"e1d5de81fabf4e784330bb89b3dc8adf7ac34c7b","unresolved":false,"context_lines":[{"line_number":1669,"context_line":""},{"line_number":1670,"context_line":"        # Running contract before migrate to a higher version should raise an"},{"line_number":1671,"context_line":"        # exception."},{"line_number":1672,"context_line":"        try:"},{"line_number":1673,"context_line":"            self.contract(2)"},{"line_number":1674,"context_line":"        except Exception as e:"},{"line_number":1675,"context_line":"            self.assertIn(\u0027contract_repo is greater than\u0027, repr(e))"},{"line_number":1676,"context_line":"        else:"},{"line_number":1677,"context_line":"            self.fail(\u0027Running upgrades out of order did not raise a conflict\u0027)"},{"line_number":1678,"context_line":""},{"line_number":1679,"context_line":"    def test_db_sync_check(self):"},{"line_number":1680,"context_line":"        checker \u003d cli.DbSync()"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba2be162_74ec9f8f","line":1677,"range":{"start_line":1672,"start_character":0,"end_line":1677,"end_character":79},"in_reply_to":"ba2be162_d89f474e","updated":"2017-03-02 16:07:17.000000000","message":"Good idea, added the check above to assert that both the exception and the correct message are raised with the corresponding command","commit_id":"3c206e1d68500a66e1534aaef7c0e315febb98c4"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"aa3648c0ba939874c2c5241fffc4990569238fd1","unresolved":false,"context_lines":[{"line_number":1669,"context_line":""},{"line_number":1670,"context_line":"        # Running contract before migrate to a higher version should raise an"},{"line_number":1671,"context_line":"        # exception."},{"line_number":1672,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1673,"context_line":"        try:"},{"line_number":1674,"context_line":"            self.contract(2)"},{"line_number":1675,"context_line":"        except db_exception.DBMigrationError as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba2be162_bb53636a","line":1672,"updated":"2017-03-02 16:45:28.000000000","message":"also check that self.migrate(2) fails?","commit_id":"326ba5fe9a57d89ad5fac4524cad89901f5d9372"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"222c050866fb33d4c7ca1e74eea584b98bdbded4","unresolved":false,"context_lines":[{"line_number":1669,"context_line":""},{"line_number":1670,"context_line":"        # Running contract before migrate to a higher version should raise an"},{"line_number":1671,"context_line":"        # exception."},{"line_number":1672,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1673,"context_line":"        try:"},{"line_number":1674,"context_line":"            self.contract(2)"},{"line_number":1675,"context_line":"        except db_exception.DBMigrationError as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba2be162_1a38b7ee","line":1672,"in_reply_to":"ba2be162_bb53636a","updated":"2017-03-03 16:26:12.000000000","message":"Done","commit_id":"326ba5fe9a57d89ad5fac4524cad89901f5d9372"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"aa3648c0ba939874c2c5241fffc4990569238fd1","unresolved":false,"context_lines":[{"line_number":1670,"context_line":"        # Running contract before migrate to a higher version should raise an"},{"line_number":1671,"context_line":"        # exception."},{"line_number":1672,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1673,"context_line":"        try:"},{"line_number":1674,"context_line":"            self.contract(2)"},{"line_number":1675,"context_line":"        except db_exception.DBMigrationError as e:"},{"line_number":1676,"context_line":"            self.assertIn(\u0027contract_repo is greater than\u0027, repr(e))"},{"line_number":1677,"context_line":"        else:"},{"line_number":1678,"context_line":"            self.fail(\u0027Running upgrades out of order did not raise a conflict\u0027)"},{"line_number":1679,"context_line":""},{"line_number":1680,"context_line":"    def test_db_sync_check(self):"},{"line_number":1681,"context_line":"        checker \u003d cli.DbSync()"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba2be162_3b67734e","line":1678,"range":{"start_line":1673,"start_character":0,"end_line":1678,"end_character":79},"updated":"2017-03-02 16:45:28.000000000","message":"don\u0027t think this is necessary, since it is being checked in the line above.","commit_id":"326ba5fe9a57d89ad5fac4524cad89901f5d9372"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"222c050866fb33d4c7ca1e74eea584b98bdbded4","unresolved":false,"context_lines":[{"line_number":1670,"context_line":"        # Running contract before migrate to a higher version should raise an"},{"line_number":1671,"context_line":"        # exception."},{"line_number":1672,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1673,"context_line":"        try:"},{"line_number":1674,"context_line":"            self.contract(2)"},{"line_number":1675,"context_line":"        except db_exception.DBMigrationError as e:"},{"line_number":1676,"context_line":"            self.assertIn(\u0027contract_repo is greater than\u0027, repr(e))"},{"line_number":1677,"context_line":"        else:"},{"line_number":1678,"context_line":"            self.fail(\u0027Running upgrades out of order did not raise a conflict\u0027)"},{"line_number":1679,"context_line":""},{"line_number":1680,"context_line":"    def test_db_sync_check(self):"},{"line_number":1681,"context_line":"        checker \u003d cli.DbSync()"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba2be162_3a39fbf2","line":1678,"range":{"start_line":1673,"start_character":0,"end_line":1678,"end_character":79},"in_reply_to":"ba2be162_3b67734e","updated":"2017-03-03 16:26:12.000000000","message":"Done","commit_id":"326ba5fe9a57d89ad5fac4524cad89901f5d9372"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":1659,"context_line":"        # Upgrade the legacy repository"},{"line_number":1660,"context_line":"        self.upgrade()"},{"line_number":1661,"context_line":""},{"line_number":1662,"context_line":"    def test_that_running_upgrades_out_of_order_passes(self):"},{"line_number":1663,"context_line":"        # Currently, an operator can run db_sync operations out of order"},{"line_number":1664,"context_line":"        # without an error or exception being raised. If this gets fixed in"},{"line_number":1665,"context_line":"        # the future to force expand, migrate, contract to be run in order,"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_54833156","side":"PARENT","line":1662,"updated":"2017-03-08 20:37:49.000000000","message":"As far as I can tell, you can\u0027t make keystone do things out of order if you use keystone-manage. Only the unit tests are able to reach past the validation done by keystone-manage to force upgrades out of order, so this patch shifts that validation to be done later?","commit_id":"724c9b7cd91a97fca8af7f7ec4ec8ef3d450681b"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":1659,"context_line":"        # Upgrade the legacy repository"},{"line_number":1660,"context_line":"        self.upgrade()"},{"line_number":1661,"context_line":""},{"line_number":1662,"context_line":"    def _set_db_sync_command_versions(self, version\u003d1):"},{"line_number":1663,"context_line":"        self.expand(1)"},{"line_number":1664,"context_line":"        self.migrate(1)"},{"line_number":1665,"context_line":"        self.contract(1)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_74d4b546","line":1662,"updated":"2017-03-08 20:37:49.000000000","message":"The optional argument is unused.","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":1659,"context_line":"        # Upgrade the legacy repository"},{"line_number":1660,"context_line":"        self.upgrade()"},{"line_number":1661,"context_line":""},{"line_number":1662,"context_line":"    def _set_db_sync_command_versions(self, version\u003d1):"},{"line_number":1663,"context_line":"        self.expand(1)"},{"line_number":1664,"context_line":"        self.migrate(1)"},{"line_number":1665,"context_line":"        self.contract(1)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_1731bb05","line":1662,"in_reply_to":"9a30ddce_74d4b546","updated":"2017-03-08 21:56:40.000000000","message":"Done","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":1663,"context_line":"        self.expand(1)"},{"line_number":1664,"context_line":"        self.migrate(1)"},{"line_number":1665,"context_line":"        self.contract(1)"},{"line_number":1666,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027expand_repo\u0027), 1)"},{"line_number":1667,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1668,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1669,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_14e9b914","line":1666,"updated":"2017-03-08 20:37:49.000000000","message":"This is asserting that the expand repo version is non-zero, and if it\u0027s zero, then it\u0027s going to print \"1\" in the unit test logs.","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":1663,"context_line":"        self.expand(1)"},{"line_number":1664,"context_line":"        self.migrate(1)"},{"line_number":1665,"context_line":"        self.contract(1)"},{"line_number":1666,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027expand_repo\u0027), 1)"},{"line_number":1667,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1668,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1669,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_372cffaa","line":1666,"in_reply_to":"9a30ddce_14e9b914","updated":"2017-03-08 21:56:40.000000000","message":"Done","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":1671,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1672,"context_line":"        legacy_version \u003d upgrades.get_db_version()"},{"line_number":1673,"context_line":"        # Running expand ahead of legacy to a higher version should raise an"},{"line_number":1674,"context_line":"        # exception."},{"line_number":1675,"context_line":"        self.assertRaises("},{"line_number":1676,"context_line":"            db_exception.DBMigrationError, self.expand, (legacy_version + 1))"},{"line_number":1677,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_f447c51a","line":1674,"updated":"2017-03-08 20:37:49.000000000","message":"Comment is redundant with the method name.","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":1671,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1672,"context_line":"        legacy_version \u003d upgrades.get_db_version()"},{"line_number":1673,"context_line":"        # Running expand ahead of legacy to a higher version should raise an"},{"line_number":1674,"context_line":"        # exception."},{"line_number":1675,"context_line":"        self.assertRaises("},{"line_number":1676,"context_line":"            db_exception.DBMigrationError, self.expand, (legacy_version + 1))"},{"line_number":1677,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_8aa6620e","line":1674,"in_reply_to":"9a30ddce_f447c51a","updated":"2017-03-08 21:56:40.000000000","message":"Done","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":1673,"context_line":"        # Running expand ahead of legacy to a higher version should raise an"},{"line_number":1674,"context_line":"        # exception."},{"line_number":1675,"context_line":"        self.assertRaises("},{"line_number":1676,"context_line":"            db_exception.DBMigrationError, self.expand, (legacy_version + 1))"},{"line_number":1677,"context_line":""},{"line_number":1678,"context_line":"    def test_running_db_sync_migrate_ahead_of_contract_fails(self):"},{"line_number":1679,"context_line":"        self._set_db_sync_command_versions()"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_d4c5618e","line":1676,"updated":"2017-03-08 20:37:49.000000000","message":"extra parens","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":1673,"context_line":"        # Running expand ahead of legacy to a higher version should raise an"},{"line_number":1674,"context_line":"        # exception."},{"line_number":1675,"context_line":"        self.assertRaises("},{"line_number":1676,"context_line":"            db_exception.DBMigrationError, self.expand, (legacy_version + 1))"},{"line_number":1677,"context_line":""},{"line_number":1678,"context_line":"    def test_running_db_sync_migrate_ahead_of_contract_fails(self):"},{"line_number":1679,"context_line":"        self._set_db_sync_command_versions()"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_cab05a46","line":1676,"in_reply_to":"9a30ddce_d4c5618e","updated":"2017-03-08 21:56:40.000000000","message":"Done","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":1678,"context_line":"    def test_running_db_sync_migrate_ahead_of_contract_fails(self):"},{"line_number":1679,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1680,"context_line":"        # Running migrate ahead of expand to a higher version should raise an"},{"line_number":1681,"context_line":"        # exception."},{"line_number":1682,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.migrate, 2)"},{"line_number":1683,"context_line":""},{"line_number":1684,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_9442090f","line":1681,"updated":"2017-03-08 20:37:49.000000000","message":"Comment is redundant with the method name.","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":1678,"context_line":"    def test_running_db_sync_migrate_ahead_of_contract_fails(self):"},{"line_number":1679,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1680,"context_line":"        # Running migrate ahead of expand to a higher version should raise an"},{"line_number":1681,"context_line":"        # exception."},{"line_number":1682,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.migrate, 2)"},{"line_number":1683,"context_line":""},{"line_number":1684,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_eaab5ed4","line":1681,"in_reply_to":"9a30ddce_9442090f","updated":"2017-03-08 21:56:40.000000000","message":"Done","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"eb18cf1ecd090e2aa35e8f89d29e8b6c66704846","unresolved":false,"context_lines":[{"line_number":1684,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"},{"line_number":1685,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1686,"context_line":"        # Running contract ahead of migrate to a higher version should raise an"},{"line_number":1687,"context_line":"        # exception."},{"line_number":1688,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1689,"context_line":""},{"line_number":1690,"context_line":"    def test_db_sync_check(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_743bd59c","line":1687,"updated":"2017-03-08 20:37:49.000000000","message":"Comment is redundant with the method name.","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"ee10d84304878b37ffa396063dbe13ac4c3bfdbc","unresolved":false,"context_lines":[{"line_number":1684,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"},{"line_number":1685,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1686,"context_line":"        # Running contract ahead of migrate to a higher version should raise an"},{"line_number":1687,"context_line":"        # exception."},{"line_number":1688,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1689,"context_line":""},{"line_number":1690,"context_line":"    def test_db_sync_check(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a30ddce_0abb5228","line":1687,"in_reply_to":"9a30ddce_743bd59c","updated":"2017-03-08 21:56:40.000000000","message":"Done","commit_id":"e632e5009f8f192bf7b2c1914bf6b2882c526273"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"e167b5c0951cdeb6e46086707e9958c6ae49e6ee","unresolved":false,"context_lines":[{"line_number":1754,"context_line":"        log_info \u003d self.useFixture(fixtures.FakeLogger(level\u003dlog.INFO))"},{"line_number":1755,"context_line":"        status \u003d checker.check_db_sync_status()"},{"line_number":1756,"context_line":"        self.assertIn(\"Your database is out of sync\", log_info.output)"},{"line_number":1757,"context_line":"        self.assertEqual(status, 1)"},{"line_number":1758,"context_line":""},{"line_number":1759,"context_line":"    def test_migration_002_password_created_at_not_nullable(self):"},{"line_number":1760,"context_line":"        # upgrade each repository to 001"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_02dcc471","side":"PARENT","line":1757,"updated":"2017-03-10 16:10:23.000000000","message":"Why remove this test? Don\u0027t we still want --check to work and raise this alert?","commit_id":"7048b6759b7a25592555ab206c1767a2243ac9b0"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"02c54a7ebd0c091b4a9eb20a22b4ccf176bf6862","unresolved":false,"context_lines":[{"line_number":1754,"context_line":"        log_info \u003d self.useFixture(fixtures.FakeLogger(level\u003dlog.INFO))"},{"line_number":1755,"context_line":"        status \u003d checker.check_db_sync_status()"},{"line_number":1756,"context_line":"        self.assertIn(\"Your database is out of sync\", log_info.output)"},{"line_number":1757,"context_line":"        self.assertEqual(status, 1)"},{"line_number":1758,"context_line":""},{"line_number":1759,"context_line":"    def test_migration_002_password_created_at_not_nullable(self):"},{"line_number":1760,"context_line":"        # upgrade each repository to 001"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_9e7f24df","side":"PARENT","line":1757,"in_reply_to":"9a30ddce_02dcc471","updated":"2017-03-10 19:49:59.000000000","message":"+1 I was wondering this, too. I don\u0027t see anything invalid about this test after this patch?","commit_id":"7048b6759b7a25592555ab206c1767a2243ac9b0"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"8c79f4a007319e95f32999ce9008385cd7de09f5","unresolved":false,"context_lines":[{"line_number":1754,"context_line":"        log_info \u003d self.useFixture(fixtures.FakeLogger(level\u003dlog.INFO))"},{"line_number":1755,"context_line":"        status \u003d checker.check_db_sync_status()"},{"line_number":1756,"context_line":"        self.assertIn(\"Your database is out of sync\", log_info.output)"},{"line_number":1757,"context_line":"        self.assertEqual(status, 1)"},{"line_number":1758,"context_line":""},{"line_number":1759,"context_line":"    def test_migration_002_password_created_at_not_nullable(self):"},{"line_number":1760,"context_line":"        # upgrade each repository to 001"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a30ddce_9c8bac1e","side":"PARENT","line":1757,"in_reply_to":"9a30ddce_9e7f24df","updated":"2017-03-10 21:28:45.000000000","message":"Ahh. Well ultimately this patch makes the first if statement check in --check redundant and unnecessary so I had removed it and with it, this test. However, after adding it back I forgot to re-add this test as well. Will change back","commit_id":"7048b6759b7a25592555ab206c1767a2243ac9b0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"50ec347b6be4971754ca35be374915f9d8817b4e","unresolved":false,"context_lines":[{"line_number":1754,"context_line":"        log_info \u003d self.useFixture(fixtures.FakeLogger(level\u003dlog.INFO))"},{"line_number":1755,"context_line":"        status \u003d checker.check_db_sync_status()"},{"line_number":1756,"context_line":"        self.assertIn(\"Your database is out of sync\", log_info.output)"},{"line_number":1757,"context_line":"        self.assertEqual(status, 1)"},{"line_number":1758,"context_line":""},{"line_number":1759,"context_line":"    def test_migration_002_password_created_at_not_nullable(self):"},{"line_number":1760,"context_line":"        # upgrade each repository to 001"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a30ddce_819240f4","side":"PARENT","line":1757,"updated":"2017-03-13 14:47:33.000000000","message":"Instead of removing the test completely we should see if we can assert an exception is raised when contracting the database. It looks a little strange to remove this test in a patch that enforces upgrade order.\n\nAlso - we could keep it, catch the exception, and double check to make sure things are still logged. Operators running this will likely check the logs if they hit this type of situation.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2105c8d7ebe5bf1c32b6dc9b201f1c50bd50b3a8","unresolved":false,"context_lines":[{"line_number":1754,"context_line":"        log_info \u003d self.useFixture(fixtures.FakeLogger(level\u003dlog.INFO))"},{"line_number":1755,"context_line":"        status \u003d checker.check_db_sync_status()"},{"line_number":1756,"context_line":"        self.assertIn(\"Your database is out of sync\", log_info.output)"},{"line_number":1757,"context_line":"        self.assertEqual(status, 1)"},{"line_number":1758,"context_line":""},{"line_number":1759,"context_line":"    def test_migration_002_password_created_at_not_nullable(self):"},{"line_number":1760,"context_line":"        # upgrade each repository to 001"}],"source_content_type":"text/x-python","patch_set":14,"id":"ffe62b97_45da7517","side":"PARENT","line":1757,"in_reply_to":"9a30ddce_427a06be","updated":"2017-03-24 15:17:51.000000000","message":"Yeah - either way it would be nice to have the behavior documented in a test somewhere. We should just show that it is done somehow so that we know we won\u0027t regress.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"2ac77279ce50ad35101d20b6dda41405a20d866e","unresolved":false,"context_lines":[{"line_number":1754,"context_line":"        log_info \u003d self.useFixture(fixtures.FakeLogger(level\u003dlog.INFO))"},{"line_number":1755,"context_line":"        status \u003d checker.check_db_sync_status()"},{"line_number":1756,"context_line":"        self.assertIn(\"Your database is out of sync\", log_info.output)"},{"line_number":1757,"context_line":"        self.assertEqual(status, 1)"},{"line_number":1758,"context_line":""},{"line_number":1759,"context_line":"    def test_migration_002_password_created_at_not_nullable(self):"},{"line_number":1760,"context_line":"        # upgrade each repository to 001"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a30ddce_427a06be","side":"PARENT","line":1757,"in_reply_to":"9a30ddce_819240f4","updated":"2017-03-13 15:24:07.000000000","message":"If we do a try catch, however, it will still be returning the message from the new validation method and therefore not really be testing db_sync check since it will fail inside validate_upgrade_order long before it can in db_sync check right? Unless of course I just start mocking things","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"83ac29f9d57cfc965740f6efc215aaa343cfa453","unresolved":false,"context_lines":[{"line_number":1745,"context_line":"        self.assertIn(\"All db_sync commands are upgraded\", log_info.output)"},{"line_number":1746,"context_line":"        self.assertEqual(status, 0)"},{"line_number":1747,"context_line":""},{"line_number":1748,"context_line":"    def test_db_sync_check_out_of_sync(self):"},{"line_number":1749,"context_line":"        checker \u003d cli.DbSync()"},{"line_number":1750,"context_line":"        # Assert we alert operator upgrades are out of sync"},{"line_number":1751,"context_line":"        self.expand(3)"}],"source_content_type":"text/x-python","patch_set":15,"id":"ffe62b97_25fad141","side":"PARENT","line":1748,"updated":"2017-03-24 15:22:30.000000000","message":"This is essentially the same test as the one on line 1715 now, just with different repository versions. Keeping the logging would be useful though. Since this case is covered in the above tests - I\u0027d be fine removing this one test in favor of three other tests that test more cases.","commit_id":"cd642d2d4e67f60712bbbac87d81ecfcae292fbb"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"b2c6a4417a5efc0e5c7116712b01b952ef2f4445","unresolved":false,"context_lines":[{"line_number":1745,"context_line":"        self.assertIn(\"All db_sync commands are upgraded\", log_info.output)"},{"line_number":1746,"context_line":"        self.assertEqual(status, 0)"},{"line_number":1747,"context_line":""},{"line_number":1748,"context_line":"    def test_db_sync_check_out_of_sync(self):"},{"line_number":1749,"context_line":"        checker \u003d cli.DbSync()"},{"line_number":1750,"context_line":"        # Assert we alert operator upgrades are out of sync"},{"line_number":1751,"context_line":"        self.expand(3)"}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_1271fc0c","side":"PARENT","line":1748,"updated":"2017-03-27 19:48:19.000000000","message":"-1 This test is absolutely still relevant. This condition should be unreachable, but also trigger some sort of critical failure on any db_sync operation other than --check.","commit_id":"c7e29560b7bf7a44e44722eea0645bf18ad56af3"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"84205ee5f5cfcb54cbcba95a58e4964de416e58a","unresolved":false,"context_lines":[{"line_number":1745,"context_line":"        self.assertIn(\"All db_sync commands are upgraded\", log_info.output)"},{"line_number":1746,"context_line":"        self.assertEqual(status, 0)"},{"line_number":1747,"context_line":""},{"line_number":1748,"context_line":"    def test_db_sync_check_out_of_sync(self):"},{"line_number":1749,"context_line":"        checker \u003d cli.DbSync()"},{"line_number":1750,"context_line":"        # Assert we alert operator upgrades are out of sync"},{"line_number":1751,"context_line":"        self.expand(3)"}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_617cf01e","side":"PARENT","line":1748,"in_reply_to":"ffe62b97_1271fc0c","updated":"2017-03-27 21:54:17.000000000","message":"The condition \u0027IS\u0027 unreachable, and the \u0027critical failure\u0027 it triggers on any db_sync is db_exception.DBMigrationError which is already being tested in the unit tests added. Elaborate?","commit_id":"c7e29560b7bf7a44e44722eea0645bf18ad56af3"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"b2c6a4417a5efc0e5c7116712b01b952ef2f4445","unresolved":false,"context_lines":[{"line_number":1698,"context_line":"        self.expand(1)"},{"line_number":1699,"context_line":"        self.migrate(1)"},{"line_number":1700,"context_line":"        self.contract(1)"},{"line_number":1701,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027expand_repo\u0027), 1)"},{"line_number":1702,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1703,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1704,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_12d57ce5","line":1701,"updated":"2017-03-27 19:48:19.000000000","message":"-1 As mentioned in a previous review, using assertTrue here does not make any sense. \"1\" becomes the error message when the repo version is falsey, which (IIRC) will never happen because they\u0027re strings, starting with \"0\" which is already truthy.","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"144fc2f7276d2f3dd7d5b6edf1771d91d7a40fa5","unresolved":false,"context_lines":[{"line_number":1698,"context_line":"        self.expand(1)"},{"line_number":1699,"context_line":"        self.migrate(1)"},{"line_number":1700,"context_line":"        self.contract(1)"},{"line_number":1701,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027expand_repo\u0027), 1)"},{"line_number":1702,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1703,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1704,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_92ecec8f","line":1701,"in_reply_to":"ffe62b97_12d57ce5","updated":"2017-03-27 19:49:48.000000000","message":"caveat emptor: my assumption that it\u0027s a string may be incorrect, but the assertion is still wrong.","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"84205ee5f5cfcb54cbcba95a58e4964de416e58a","unresolved":false,"context_lines":[{"line_number":1698,"context_line":"        self.expand(1)"},{"line_number":1699,"context_line":"        self.migrate(1)"},{"line_number":1700,"context_line":"        self.contract(1)"},{"line_number":1701,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027expand_repo\u0027), 1)"},{"line_number":1702,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1703,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1704,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_01ebe412","line":1701,"in_reply_to":"ffe62b97_92ecec8f","updated":"2017-03-27 21:54:17.000000000","message":"Correct on the assumption of incorrect. Just setting versions to 1. Not sure how 1 becomes an error message since this isn\u0027t being used as a return for validation. This is only setting expand, migrate, contract versions for the following unit tests and verifying (as a sanity check) that they all have been set to 1.","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"b2c6a4417a5efc0e5c7116712b01b952ef2f4445","unresolved":false,"context_lines":[{"line_number":1706,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1707,"context_line":"        legacy_version \u003d upgrades.get_db_version()"},{"line_number":1708,"context_line":"        self.assertRaises("},{"line_number":1709,"context_line":"            db_exception.DBMigrationError, self.expand, legacy_version + 1)"},{"line_number":1710,"context_line":""},{"line_number":1711,"context_line":"    def test_running_db_sync_migrate_ahead_of_contract_fails(self):"},{"line_number":1712,"context_line":"        self._set_db_sync_command_versions()"}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_d27f14de","line":1709,"updated":"2017-03-27 19:48:19.000000000","message":"-1 The legacy version absolutely does not matter to expand, only that all legacy migrations have already been run.","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"84205ee5f5cfcb54cbcba95a58e4964de416e58a","unresolved":false,"context_lines":[{"line_number":1706,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1707,"context_line":"        legacy_version \u003d upgrades.get_db_version()"},{"line_number":1708,"context_line":"        self.assertRaises("},{"line_number":1709,"context_line":"            db_exception.DBMigrationError, self.expand, legacy_version + 1)"},{"line_number":1710,"context_line":""},{"line_number":1711,"context_line":"    def test_running_db_sync_migrate_ahead_of_contract_fails(self):"},{"line_number":1712,"context_line":"        self._set_db_sync_command_versions()"}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_e1536016","line":1709,"in_reply_to":"ffe62b97_d27f14de","updated":"2017-03-27 21:54:17.000000000","message":"Done","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"b2c6a4417a5efc0e5c7116712b01b952ef2f4445","unresolved":false,"context_lines":[{"line_number":1708,"context_line":"        self.assertRaises("},{"line_number":1709,"context_line":"            db_exception.DBMigrationError, self.expand, legacy_version + 1)"},{"line_number":1710,"context_line":""},{"line_number":1711,"context_line":"    def test_running_db_sync_migrate_ahead_of_contract_fails(self):"},{"line_number":1712,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1713,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.migrate, 2)"},{"line_number":1714,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_724048a6","line":1711,"updated":"2017-03-27 19:48:19.000000000","message":"-1 Data migration should be allowed to run ahead of schema contraction.","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"84205ee5f5cfcb54cbcba95a58e4964de416e58a","unresolved":false,"context_lines":[{"line_number":1708,"context_line":"        self.assertRaises("},{"line_number":1709,"context_line":"            db_exception.DBMigrationError, self.expand, legacy_version + 1)"},{"line_number":1710,"context_line":""},{"line_number":1711,"context_line":"    def test_running_db_sync_migrate_ahead_of_contract_fails(self):"},{"line_number":1712,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1713,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.migrate, 2)"},{"line_number":1714,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"ffe62b97_014f646b","line":1711,"in_reply_to":"ffe62b97_724048a6","updated":"2017-03-27 21:54:17.000000000","message":"ah, in reality its the spelling of the test. s/contract/expand/","commit_id":"2304da1ee382ab9cc9fb0bf6e11bf8d97524ad8a"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"23d713f677c634e071fd36698b75645aba1fc6f1","unresolved":false,"context_lines":[{"line_number":1700,"context_line":"        self.contract(1)"},{"line_number":1701,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027expand_repo\u0027), 1)"},{"line_number":1702,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1703,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1704,"context_line":""},{"line_number":1705,"context_line":"    def test_running_db_sync_migrate_ahead_of_expand_fails(self):"},{"line_number":1706,"context_line":"        self._set_db_sync_command_versions()"}],"source_content_type":"text/x-python","patch_set":20,"id":"ffe62b97_08dbe77b","line":1703,"updated":"2017-03-28 14:04:02.000000000","message":"-1 Seriously, you\u0027re using assertTrue here. This is not what you want, and the old tests were similarly broken.","commit_id":"ca9e958e155c8d2eef40ddde041ed5709e27a315"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"d79c099ba3bcd3971bb74938dc79f5fbab6dc6c5","unresolved":false,"context_lines":[{"line_number":1700,"context_line":"        self.contract(1)"},{"line_number":1701,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027expand_repo\u0027), 1)"},{"line_number":1702,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027data_migration_repo\u0027), 1)"},{"line_number":1703,"context_line":"        self.assertTrue(upgrades.get_db_version(\u0027contract_repo\u0027), 1)"},{"line_number":1704,"context_line":""},{"line_number":1705,"context_line":"    def test_running_db_sync_migrate_ahead_of_expand_fails(self):"},{"line_number":1706,"context_line":"        self._set_db_sync_command_versions()"}],"source_content_type":"text/x-python","patch_set":20,"id":"ffe62b97_9842a6aa","line":1703,"in_reply_to":"ffe62b97_08dbe77b","updated":"2017-03-28 14:56:24.000000000","message":"Done","commit_id":"ca9e958e155c8d2eef40ddde041ed5709e27a315"},{"author":{"_account_id":4,"name":"Dolph Mathews","email":"dolph.mathews@gmail.com","username":"dolph"},"change_message_id":"23d713f677c634e071fd36698b75645aba1fc6f1","unresolved":false,"context_lines":[{"line_number":1708,"context_line":""},{"line_number":1709,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"},{"line_number":1710,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1711,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1712,"context_line":""},{"line_number":1713,"context_line":"    def test_db_sync_check(self):"},{"line_number":1714,"context_line":"        checker \u003d cli.DbSync()"}],"source_content_type":"text/x-python","patch_set":20,"id":"ffe62b97_48ad6f99","line":1711,"updated":"2017-03-28 14:04:02.000000000","message":"You\u0027re missing a test where you try to run expand without first finishing legacy migrations.","commit_id":"ca9e958e155c8d2eef40ddde041ed5709e27a315"},{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"6daca59e929946f2836ff03d49533b83d2c11b9b","unresolved":false,"context_lines":[{"line_number":1717,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"},{"line_number":1718,"context_line":"        self.upgrade()"},{"line_number":1719,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1720,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":""},{"line_number":1723,"context_line":"class FullMigration(SqlMigrateBase, unit.TestCase):"}],"source_content_type":"text/x-python","patch_set":28,"id":"bff0334d_0ebda964","line":1720,"updated":"2017-04-06 12:11:53.000000000","message":"is this just running contact without expand+migrate?\n\nit\u0027d be great if were exhaustive on these tests:\n\n    just migrate\n    just contract\n    run expand then contract\n\ncan you please add the missing test case?\n\nalso, what if we check on the error message being given? would that be good since we\u0027ll be sure what\u0027s the reason of the DBMigrationError?","commit_id":"ea91a90a54794734ec92ac76397b7519a9f86fc1"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"114520c800d3b2571fa1291dbfcb03299f47648c","unresolved":false,"context_lines":[{"line_number":1717,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"},{"line_number":1718,"context_line":"        self.upgrade()"},{"line_number":1719,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1720,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":""},{"line_number":1723,"context_line":"class FullMigration(SqlMigrateBase, unit.TestCase):"}],"source_content_type":"text/x-python","patch_set":28,"id":"bff0334d_68e9c518","line":1720,"in_reply_to":"bff0334d_0ebda964","updated":"2017-04-06 14:53:33.000000000","message":"_set_db_sync_sommand_versions is running expand + migrate","commit_id":"ea91a90a54794734ec92ac76397b7519a9f86fc1"},{"author":{"_account_id":18464,"name":"Richard Avelar","email":"csravelar@gmail.com","username":"razone"},"change_message_id":"55dbb2cce17fd31ce90a36084fdaa950317eff9b","unresolved":false,"context_lines":[{"line_number":1717,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"},{"line_number":1718,"context_line":"        self.upgrade()"},{"line_number":1719,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1720,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":""},{"line_number":1723,"context_line":"class FullMigration(SqlMigrateBase, unit.TestCase):"}],"source_content_type":"text/x-python","patch_set":28,"id":"bff0334d_3a7ce3b3","line":1720,"in_reply_to":"bff0334d_527e9c19","updated":"2017-04-07 20:20:59.000000000","message":"Yes, essentially, they all run expand, migrate, contract in order to initially set them to version 1. Then we simply test that migrate cannot be upgraded to 2, and contract cannot be upgraded to 2.","commit_id":"ea91a90a54794734ec92ac76397b7519a9f86fc1"},{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"a4d847c652c79a954549d7040645e234379b0c84","unresolved":false,"context_lines":[{"line_number":1717,"context_line":"    def test_running_db_sync_contract_ahead_of_migrate_fails(self):"},{"line_number":1718,"context_line":"        self.upgrade()"},{"line_number":1719,"context_line":"        self._set_db_sync_command_versions()"},{"line_number":1720,"context_line":"        self.assertRaises(db_exception.DBMigrationError, self.contract, 2)"},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":""},{"line_number":1723,"context_line":"class FullMigration(SqlMigrateBase, unit.TestCase):"}],"source_content_type":"text/x-python","patch_set":28,"id":"bff0334d_527e9c19","line":1720,"in_reply_to":"bff0334d_68e9c518","updated":"2017-04-06 21:08:14.000000000","message":"so the test above is also running expand + migrate in _set_db_sync_command_versions ? they both call the same method","commit_id":"ea91a90a54794734ec92ac76397b7519a9f86fc1"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"3cafb4a696c0929e286dd04ae576c03e194b1a30","unresolved":false,"context_lines":[{"line_number":1745,"context_line":"        self.assertIn(\"All db_sync commands are upgraded\", log_info.output)"},{"line_number":1746,"context_line":"        self.assertEqual(status, 0)"},{"line_number":1747,"context_line":""},{"line_number":1748,"context_line":"    def test_db_sync_check_out_of_sync(self):"},{"line_number":1749,"context_line":"        checker \u003d cli.DbSync()"},{"line_number":1750,"context_line":"        # Assert we alert operator upgrades are out of sync"},{"line_number":1751,"context_line":"        self.expand(3)"}],"source_content_type":"text/x-python","patch_set":31,"id":"7ffa3b31_d602ac0b","side":"PARENT","line":1748,"updated":"2017-04-17 21:05:31.000000000","message":"I see a few different comments asking for this not to be removed. I see that you explained that this condition is no longer reachable. I think that if it\u0027s not possible to reach a condition that would satisfy this test, the code that logs the \"Your database is out of sync\" message should be removed. Otherwise, this test should stay.","commit_id":"2dbd5d99bbf71ad5c4a81f54e8d4ce0de258ab00"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"f3105cd4c5284db1f3bd040d42f3cf3153358110","unresolved":false,"context_lines":[{"line_number":1745,"context_line":"        self.assertIn(\"All db_sync commands are upgraded\", log_info.output)"},{"line_number":1746,"context_line":"        self.assertEqual(status, 0)"},{"line_number":1747,"context_line":""},{"line_number":1748,"context_line":"    def test_db_sync_check_out_of_sync(self):"},{"line_number":1749,"context_line":"        checker \u003d cli.DbSync()"},{"line_number":1750,"context_line":"        # Assert we alert operator upgrades are out of sync"},{"line_number":1751,"context_line":"        self.expand(3)"}],"source_content_type":"text/x-python","patch_set":31,"id":"5f201791_0afcd9b2","side":"PARENT","line":1748,"in_reply_to":"7ffa3b31_d602ac0b","updated":"2017-06-27 20:47:53.000000000","message":"I attempted to address this concern in the latest patch set.","commit_id":"2dbd5d99bbf71ad5c4a81f54e8d4ce0de258ab00"}]}