)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7198,"name":"Jay Bryant","email":"jungleboyj@electronicjungle.net","username":"jsbryant"},"change_message_id":"485bdbb9135bb6a2b5421808ddb23b53e5b00c80","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In Ceph when backup base is diff-format and use this backup"},{"line_number":10,"context_line":"to restore volumes not rbd, need to find out the correct restore"},{"line_number":11,"context_line":"point."},{"line_number":12,"context_line":"This is fixed in cinder/backup/drivers/ceph.py _diff_restore_allowed."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: I006c05d246f59fc6aff597543bedc68589d37576"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"ba3cc151_606ee3ca","line":11,"updated":"2015-07-07 16:41:21.000000000","message":"Should have an empty line here.\n\nAlso, it would be helpful to add what is actually changed to make the fix.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79d4750a4d8d72b2a0eb3ddb6242fd4b46955f41","unresolved":false,"context_lines":[{"line_number":10,"context_line":"to restore volumes not rbd, need to find out the correct restore"},{"line_number":11,"context_line":"point."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Before the change: when back is diff-format, and destination volume"},{"line_number":14,"context_line":"is not rbd, it returns restore_point as None. As a result, backup"},{"line_number":15,"context_line":"restore uses wrong image/snapshot to do restore."},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"ba3cc151_bcd62f4d","line":13,"updated":"2015-07-08 11:34:27.000000000","message":"s/back/backup","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"269c1636d8dfa89d15f264e0c825646d20d55ef2","unresolved":false,"context_lines":[{"line_number":10,"context_line":"to restore volumes not rbd, need to find out the correct restore"},{"line_number":11,"context_line":"point."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Before the change: when back is diff-format, and destination volume"},{"line_number":14,"context_line":"is not rbd, it returns restore_point as None. As a result, backup"},{"line_number":15,"context_line":"restore uses wrong image/snapshot to do restore."},{"line_number":16,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"ba3cc151_3cc2f185","line":13,"in_reply_to":"ba3cc151_bcd62f4d","updated":"2015-07-09 01:30:09.000000000","message":"Done","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79d4750a4d8d72b2a0eb3ddb6242fd4b46955f41","unresolved":false,"context_lines":[{"line_number":11,"context_line":"point."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Before the change: when back is diff-format, and destination volume"},{"line_number":14,"context_line":"is not rbd, it returns restore_point as None. As a result, backup"},{"line_number":15,"context_line":"restore uses wrong image/snapshot to do restore."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"With the change, if backup is diff-format, it returns the restore"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"ba3cc151_7c43c713","line":14,"updated":"2015-07-08 11:34:27.000000000","message":"\"it returns\"? Who returns restore_point as None?\n\nI think it would read better if you say something like \"not rbd, we use None as the restore_point. As a\" or moving line 21 before this paragraph so we know who is returning that.","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"269c1636d8dfa89d15f264e0c825646d20d55ef2","unresolved":false,"context_lines":[{"line_number":11,"context_line":"point."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Before the change: when back is diff-format, and destination volume"},{"line_number":14,"context_line":"is not rbd, it returns restore_point as None. As a result, backup"},{"line_number":15,"context_line":"restore uses wrong image/snapshot to do restore."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"With the change, if backup is diff-format, it returns the restore"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"ba3cc151_9c077dc9","line":14,"in_reply_to":"ba3cc151_7c43c713","updated":"2015-07-09 01:30:09.000000000","message":"Done","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79d4750a4d8d72b2a0eb3ddb6242fd4b46955f41","unresolved":false,"context_lines":[{"line_number":15,"context_line":"restore uses wrong image/snapshot to do restore."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"With the change, if backup is diff-format, it returns the restore"},{"line_number":18,"context_line":"point, so that whether differential restore or not, it uses the"},{"line_number":19,"context_line":"correct image/snapshot to do restore."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"This is fixed in cinder/backup/drivers/ceph.py _diff_restore_allowed."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"ba3cc151_fc4fd731","line":18,"updated":"2015-07-08 11:34:27.000000000","message":"Doesn\u0027t sound too good the \"so that whether differential restore or not\" part. I think you could phrase it a little bit better, as it is a little confusing IMO.\n\nMy suggestion, something on the lines of: \"we always get the right restore point, regardless of the kind of restore we\u0027ll do, differential or not.\"","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"269c1636d8dfa89d15f264e0c825646d20d55ef2","unresolved":false,"context_lines":[{"line_number":15,"context_line":"restore uses wrong image/snapshot to do restore."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"With the change, if backup is diff-format, it returns the restore"},{"line_number":18,"context_line":"point, so that whether differential restore or not, it uses the"},{"line_number":19,"context_line":"correct image/snapshot to do restore."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"This is fixed in cinder/backup/drivers/ceph.py _diff_restore_allowed."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"ba3cc151_3ccc3188","line":18,"in_reply_to":"ba3cc151_fc4fd731","updated":"2015-07-09 01:30:09.000000000","message":"Done","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"}],"cinder/backup/drivers/ceph.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"2e4107662cce146245b1e6a2991f061230411c49","unresolved":false,"context_lines":[{"line_number":1033,"context_line":""},{"line_number":1034,"context_line":"    def _diff_restore_allowed(self, base_name, backup, volume, volume_file,"},{"line_number":1035,"context_line":"                              rados_client):"},{"line_number":1036,"context_line":"        \"\"\"Determine whether a differential restore is possible/allowed,"},{"line_number":1037,"context_line":"        and find out the restore points if backup base is diff-format"},{"line_number":1038,"context_line":""},{"line_number":1039,"context_line":"        In order for a differential restore to be performed we need:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_b420c3b1","line":1036,"updated":"2015-07-07 10:56:41.000000000","message":"First line must be a summary that fits in 1 line and ends with a .\n\nhttps://www.python.org/dev/peps/pep-0257/#multi-line-docstrings","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"84af3f42636e47010268a29107a88f66a286bed2","unresolved":false,"context_lines":[{"line_number":1033,"context_line":""},{"line_number":1034,"context_line":"    def _diff_restore_allowed(self, base_name, backup, volume, volume_file,"},{"line_number":1035,"context_line":"                              rados_client):"},{"line_number":1036,"context_line":"        \"\"\"Determine whether a differential restore is possible/allowed,"},{"line_number":1037,"context_line":"        and find out the restore points if backup base is diff-format"},{"line_number":1038,"context_line":""},{"line_number":1039,"context_line":"        In order for a differential restore to be performed we need:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_99902fef","line":1036,"in_reply_to":"ba3cc151_b420c3b1","updated":"2015-07-08 02:23:33.000000000","message":"Thank you very much, Gorka. Seems you teach me a lot. ^_^ I like your comments.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"2e4107662cce146245b1e6a2991f061230411c49","unresolved":false,"context_lines":[{"line_number":1036,"context_line":"        \"\"\"Determine whether a differential restore is possible/allowed,"},{"line_number":1037,"context_line":"        and find out the restore points if backup base is diff-format"},{"line_number":1038,"context_line":""},{"line_number":1039,"context_line":"        In order for a differential restore to be performed we need:"},{"line_number":1040,"context_line":"            * destination volume must be RBD"},{"line_number":1041,"context_line":"            * destination volume must have zero extents"},{"line_number":1042,"context_line":"            * backup base image must exist"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_072ce348","line":1039,"updated":"2015-07-07 10:56:41.000000000","message":"In this list there\u0027s no mention of the volume being different than the original volume.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"84af3f42636e47010268a29107a88f66a286bed2","unresolved":false,"context_lines":[{"line_number":1036,"context_line":"        \"\"\"Determine whether a differential restore is possible/allowed,"},{"line_number":1037,"context_line":"        and find out the restore points if backup base is diff-format"},{"line_number":1038,"context_line":""},{"line_number":1039,"context_line":"        In order for a differential restore to be performed we need:"},{"line_number":1040,"context_line":"            * destination volume must be RBD"},{"line_number":1041,"context_line":"            * destination volume must have zero extents"},{"line_number":1042,"context_line":"            * backup base image must exist"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_59194787","line":1039,"in_reply_to":"ba3cc151_072ce348","updated":"2015-07-08 02:23:33.000000000","message":"Done","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"2e4107662cce146245b1e6a2991f061230411c49","unresolved":false,"context_lines":[{"line_number":1052,"context_line":"                                                       rados_client)"},{"line_number":1053,"context_line":""},{"line_number":1054,"context_line":"        if not rbd_exists:"},{"line_number":1055,"context_line":"            return not_allowed"},{"line_number":1056,"context_line":""},{"line_number":1057,"context_line":"        # Get the restore point. If no restore point is found, we assume"},{"line_number":1058,"context_line":"        # that the backup was not performed using diff/incremental methods"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_67504fd5","line":1055,"updated":"2015-07-07 10:56:41.000000000","message":"Since not_allowed is only used here I don\u0027t see the point of initializing it outside this if.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"84af3f42636e47010268a29107a88f66a286bed2","unresolved":false,"context_lines":[{"line_number":1052,"context_line":"                                                       rados_client)"},{"line_number":1053,"context_line":""},{"line_number":1054,"context_line":"        if not rbd_exists:"},{"line_number":1055,"context_line":"            return not_allowed"},{"line_number":1056,"context_line":""},{"line_number":1057,"context_line":"        # Get the restore point. If no restore point is found, we assume"},{"line_number":1058,"context_line":"        # that the backup was not performed using diff/incremental methods"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_c417ae43","line":1055,"in_reply_to":"ba3cc151_67504fd5","updated":"2015-07-08 02:23:33.000000000","message":"I agree with you.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":7198,"name":"Jay Bryant","email":"jungleboyj@electronicjungle.net","username":"jsbryant"},"change_message_id":"485bdbb9135bb6a2b5421808ddb23b53e5b00c80","unresolved":false,"context_lines":[{"line_number":1057,"context_line":"        # Get the restore point. If no restore point is found, we assume"},{"line_number":1058,"context_line":"        # that the backup was not performed using diff/incremental methods"},{"line_number":1059,"context_line":"        # so we enforce full copy."},{"line_number":1060,"context_line":""},{"line_number":1061,"context_line":"        restore_point \u003d self._get_restore_point(base_name, backup[\u0027id\u0027])"},{"line_number":1062,"context_line":""},{"line_number":1063,"context_line":"        if self._file_is_rbd(volume_file):"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_e01d1367","line":1060,"updated":"2015-07-07 16:41:21.000000000","message":"Why the added line here?","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"84af3f42636e47010268a29107a88f66a286bed2","unresolved":false,"context_lines":[{"line_number":1057,"context_line":"        # Get the restore point. If no restore point is found, we assume"},{"line_number":1058,"context_line":"        # that the backup was not performed using diff/incremental methods"},{"line_number":1059,"context_line":"        # so we enforce full copy."},{"line_number":1060,"context_line":""},{"line_number":1061,"context_line":"        restore_point \u003d self._get_restore_point(base_name, backup[\u0027id\u0027])"},{"line_number":1062,"context_line":""},{"line_number":1063,"context_line":"        if self._file_is_rbd(volume_file):"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_04127657","line":1060,"in_reply_to":"ba3cc151_e01d1367","updated":"2015-07-08 02:23:33.000000000","message":"I delete it.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"2e4107662cce146245b1e6a2991f061230411c49","unresolved":false,"context_lines":[{"line_number":1065,"context_line":"            # If the volume we are restoring to is the volume the backup was"},{"line_number":1066,"context_line":"            # made from, force a full restore since a diff will not work in"},{"line_number":1067,"context_line":"            # this case."},{"line_number":1068,"context_line":"            if volume[\u0027id\u0027] \u003d\u003d backup[\u0027volume_id\u0027]:"},{"line_number":1069,"context_line":"                LOG.debug(\"Destination volume is same as backup source volume \""},{"line_number":1070,"context_line":"                          \"%s - forcing full copy.\", volume[\u0027id\u0027])"},{"line_number":1071,"context_line":"                return False, restore_point"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_eacc26bd","line":1068,"updated":"2015-07-07 10:56:41.000000000","message":"I don\u0027t think we need to check this (we can remove lines 1068 to 1071), if it\u0027s the same volume and it has data it will have extents, so it will return the same result on line 1080 or 1089.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"84af3f42636e47010268a29107a88f66a286bed2","unresolved":false,"context_lines":[{"line_number":1065,"context_line":"            # If the volume we are restoring to is the volume the backup was"},{"line_number":1066,"context_line":"            # made from, force a full restore since a diff will not work in"},{"line_number":1067,"context_line":"            # this case."},{"line_number":1068,"context_line":"            if volume[\u0027id\u0027] \u003d\u003d backup[\u0027volume_id\u0027]:"},{"line_number":1069,"context_line":"                LOG.debug(\"Destination volume is same as backup source volume \""},{"line_number":1070,"context_line":"                          \"%s - forcing full copy.\", volume[\u0027id\u0027])"},{"line_number":1071,"context_line":"                return False, restore_point"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_e44c3244","line":1068,"in_reply_to":"ba3cc151_eacc26bd","updated":"2015-07-08 02:23:33.000000000","message":"I moved the part after \"if restore_point\", which logs different message.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79d4750a4d8d72b2a0eb3ddb6242fd4b46955f41","unresolved":false,"context_lines":[{"line_number":1033,"context_line":""},{"line_number":1034,"context_line":"    def _diff_restore_allowed(self, base_name, backup, volume, volume_file,"},{"line_number":1035,"context_line":"                              rados_client):"},{"line_number":1036,"context_line":"        \"\"\"Determine whether differential restore and restore points."},{"line_number":1037,"context_line":""},{"line_number":1038,"context_line":"        Determine whether a differential restore is possible/allowed,"},{"line_number":1039,"context_line":"        and find out the restore point if backup base is diff-format."}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_dcc1b330","line":1036,"updated":"2015-07-08 11:34:27.000000000","message":"s/whether/if\n\ns/restore and/restores is possible and\n\ns/points/point","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"269c1636d8dfa89d15f264e0c825646d20d55ef2","unresolved":false,"context_lines":[{"line_number":1033,"context_line":""},{"line_number":1034,"context_line":"    def _diff_restore_allowed(self, base_name, backup, volume, volume_file,"},{"line_number":1035,"context_line":"                              rados_client):"},{"line_number":1036,"context_line":"        \"\"\"Determine whether differential restore and restore points."},{"line_number":1037,"context_line":""},{"line_number":1038,"context_line":"        Determine whether a differential restore is possible/allowed,"},{"line_number":1039,"context_line":"        and find out the restore point if backup base is diff-format."}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_516b1ef1","line":1036,"in_reply_to":"ba3cc151_dcc1b330","updated":"2015-07-09 01:30:09.000000000","message":"Done","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79d4750a4d8d72b2a0eb3ddb6242fd4b46955f41","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"                # If the volume we are restoring to is the volume the backup"},{"line_number":1071,"context_line":"                # was made from, force a full restore since a diff will not"},{"line_number":1072,"context_line":"                # work in this case."},{"line_number":1073,"context_line":"                if volume[\u0027id\u0027] \u003d\u003d backup[\u0027volume_id\u0027]:"},{"line_number":1074,"context_line":"                    LOG.debug(\"Destination volume is same as backup source \""},{"line_number":1075,"context_line":"                              \"volume %s - forcing full copy.\", volume[\u0027id\u0027])"},{"line_number":1076,"context_line":"                    return False, restore_point"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_1c987bbf","line":1073,"updated":"2015-07-08 11:34:27.000000000","message":"I still think we don\u0027t need this case, and even though the log messages are different, it is not because the backup is the original volume that we cannot do it, but because it has extents.  If we had an empty volume with the same ID it would be possible to do it, AFAIK.","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"269c1636d8dfa89d15f264e0c825646d20d55ef2","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"                # If the volume we are restoring to is the volume the backup"},{"line_number":1071,"context_line":"                # was made from, force a full restore since a diff will not"},{"line_number":1072,"context_line":"                # work in this case."},{"line_number":1073,"context_line":"                if volume[\u0027id\u0027] \u003d\u003d backup[\u0027volume_id\u0027]:"},{"line_number":1074,"context_line":"                    LOG.debug(\"Destination volume is same as backup source \""},{"line_number":1075,"context_line":"                              \"volume %s - forcing full copy.\", volume[\u0027id\u0027])"},{"line_number":1076,"context_line":"                    return False, restore_point"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_9c28dd87","line":1073,"in_reply_to":"ba3cc151_1c987bbf","updated":"2015-07-09 01:30:09.000000000","message":"How about the case that destination volume has no extent, volumes are same, and there are data in the backup?\nI would like to find out materials and do some tests, and update it in another patch if it is as you said.","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79d4750a4d8d72b2a0eb3ddb6242fd4b46955f41","unresolved":false,"context_lines":[{"line_number":1084,"context_line":"                    return False, restore_point"},{"line_number":1085,"context_line":""},{"line_number":1086,"context_line":"                return True, restore_point"},{"line_number":1087,"context_line":"            else:"},{"line_number":1088,"context_line":"                # target volume is not rbd."},{"line_number":1089,"context_line":"                return False, restore_point"},{"line_number":1090,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_9c000b03","line":1087,"updated":"2015-07-08 11:34:27.000000000","message":"We don\u0027t need this else if you leave as method\u0027s default return as the last line, outside the ifs:\n\n return False, restore_point\n\nAnd the you can get rid of not_allowed again  ;-)","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"269c1636d8dfa89d15f264e0c825646d20d55ef2","unresolved":false,"context_lines":[{"line_number":1084,"context_line":"                    return False, restore_point"},{"line_number":1085,"context_line":""},{"line_number":1086,"context_line":"                return True, restore_point"},{"line_number":1087,"context_line":"            else:"},{"line_number":1088,"context_line":"                # target volume is not rbd."},{"line_number":1089,"context_line":"                return False, restore_point"},{"line_number":1090,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_7c587931","line":1087,"in_reply_to":"ba3cc151_9c000b03","updated":"2015-07-09 01:30:09.000000000","message":"Done","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79d4750a4d8d72b2a0eb3ddb6242fd4b46955f41","unresolved":false,"context_lines":[{"line_number":1095,"context_line":"                         \"forcing full copy.\"),"},{"line_number":1096,"context_line":"                     {\u0027backup\u0027: backup[\u0027id\u0027],"},{"line_number":1097,"context_line":"                      \u0027volume\u0027: backup[\u0027volume_id\u0027]})"},{"line_number":1098,"context_line":"            return not_allowed"},{"line_number":1099,"context_line":""},{"line_number":1100,"context_line":"    def _restore_volume(self, backup, volume, volume_file):"},{"line_number":1101,"context_line":"        \"\"\"Restore volume from backup using diff transfer if possible."}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_3c121f58","line":1098,"updated":"2015-07-08 11:34:27.000000000","message":"If you do like I mention in previous comment you don\u0027t need this return, using default return will work just fine because we know restore_point is None","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"269c1636d8dfa89d15f264e0c825646d20d55ef2","unresolved":false,"context_lines":[{"line_number":1095,"context_line":"                         \"forcing full copy.\"),"},{"line_number":1096,"context_line":"                     {\u0027backup\u0027: backup[\u0027id\u0027],"},{"line_number":1097,"context_line":"                      \u0027volume\u0027: backup[\u0027volume_id\u0027]})"},{"line_number":1098,"context_line":"            return not_allowed"},{"line_number":1099,"context_line":""},{"line_number":1100,"context_line":"    def _restore_volume(self, backup, volume, volume_file):"},{"line_number":1101,"context_line":"        \"\"\"Restore volume from backup using diff transfer if possible."}],"source_content_type":"text/x-python","patch_set":5,"id":"ba3cc151_1c44cd47","line":1098,"in_reply_to":"ba3cc151_3c121f58","updated":"2015-07-09 01:30:09.000000000","message":"Done","commit_id":"9e26089caeb9dfb23bf7b5c32c135b5e9ab9fb0e"},{"author":{"_account_id":7198,"name":"Jay Bryant","email":"jungleboyj@electronicjungle.net","username":"jsbryant"},"change_message_id":"fffdfa4d711b2c9852b0b8b736d572b2e7387152","unresolved":false,"context_lines":[{"line_number":1086,"context_line":"        else:"},{"line_number":1087,"context_line":"            LOG.info(_LI(\"No restore point found for backup\u003d\""},{"line_number":1088,"context_line":"                         \"\u0027%(backup)s\u0027 of volume %(volume)s \""},{"line_number":1089,"context_line":"                         \"although base image is found - \""},{"line_number":1090,"context_line":"                         \"forcing full copy.\"),"},{"line_number":1091,"context_line":"                     {\u0027backup\u0027: backup[\u0027id\u0027],"},{"line_number":1092,"context_line":"                      \u0027volume\u0027: backup[\u0027volume_id\u0027]})"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_62f1b200","line":1089,"updated":"2015-07-09 02:41:52.000000000","message":"Should end with \u0027.\u0027 not \u0027-\u0027","commit_id":"d507ebe61c02a604db6b07309feac0fb4b5f3363"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"7b16f36d1c8b144388fef0122b031cc98d4a1070","unresolved":false,"context_lines":[{"line_number":1086,"context_line":"        else:"},{"line_number":1087,"context_line":"            LOG.info(_LI(\"No restore point found for backup\u003d\""},{"line_number":1088,"context_line":"                         \"\u0027%(backup)s\u0027 of volume %(volume)s \""},{"line_number":1089,"context_line":"                         \"although base image is found - \""},{"line_number":1090,"context_line":"                         \"forcing full copy.\"),"},{"line_number":1091,"context_line":"                     {\u0027backup\u0027: backup[\u0027id\u0027],"},{"line_number":1092,"context_line":"                      \u0027volume\u0027: backup[\u0027volume_id\u0027]})"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_effa8a42","line":1089,"in_reply_to":"ba3cc151_62f1b200","updated":"2015-07-09 05:23:53.000000000","message":"Hi, I think \u0027-\u0027 is ok. Refer to the original code.","commit_id":"d507ebe61c02a604db6b07309feac0fb4b5f3363"},{"author":{"_account_id":8846,"name":"Hao Wang","display_name":"Hao Wang","email":"sxmatch1986@gmail.com","username":"sxmatch"},"change_message_id":"9b63ad31d5d732ff816cae63ece0de5cda809fa9","unresolved":false,"context_lines":[{"line_number":1062,"context_line":"        restore_point \u003d self._get_restore_point(base_name, backup[\u0027id\u0027])"},{"line_number":1063,"context_line":""},{"line_number":1064,"context_line":"        if restore_point:"},{"line_number":1065,"context_line":""},{"line_number":1066,"context_line":"            if self._file_is_rbd(volume_file):"},{"line_number":1067,"context_line":""},{"line_number":1068,"context_line":"                # If the volume we are restoring to is the volume the backup"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a41bdd9_8d0d6992","line":1065,"updated":"2015-07-14 02:31:28.000000000","message":"Looks like do not need this blank line.","commit_id":"c4dcad2b5e67297d23560ca10514c711b6d95dcf"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"249ae03fb2beea7f9280f3d0e7e224bfe86830da","unresolved":false,"context_lines":[{"line_number":1062,"context_line":"        restore_point \u003d self._get_restore_point(base_name, backup[\u0027id\u0027])"},{"line_number":1063,"context_line":""},{"line_number":1064,"context_line":"        if restore_point:"},{"line_number":1065,"context_line":""},{"line_number":1066,"context_line":"            if self._file_is_rbd(volume_file):"},{"line_number":1067,"context_line":""},{"line_number":1068,"context_line":"                # If the volume we are restoring to is the volume the backup"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a41bdd9_bc35fcf7","line":1065,"in_reply_to":"9a41bdd9_8d0d6992","updated":"2015-07-14 04:00:13.000000000","message":"Done","commit_id":"c4dcad2b5e67297d23560ca10514c711b6d95dcf"},{"author":{"_account_id":8846,"name":"Hao Wang","display_name":"Hao Wang","email":"sxmatch1986@gmail.com","username":"sxmatch"},"change_message_id":"9b63ad31d5d732ff816cae63ece0de5cda809fa9","unresolved":false,"context_lines":[{"line_number":1082,"context_line":"                    return False, restore_point"},{"line_number":1083,"context_line":""},{"line_number":1084,"context_line":"                return True, restore_point"},{"line_number":1085,"context_line":""},{"line_number":1086,"context_line":"        else:"},{"line_number":1087,"context_line":"            LOG.info(_LI(\"No restore point found for backup\u003d\""},{"line_number":1088,"context_line":"                         \"\u0027%(backup)s\u0027 of volume %(volume)s \""}],"source_content_type":"text/x-python","patch_set":10,"id":"9a41bdd9_4d6f612b","line":1085,"updated":"2015-07-14 02:31:28.000000000","message":"Here too.","commit_id":"c4dcad2b5e67297d23560ca10514c711b6d95dcf"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"249ae03fb2beea7f9280f3d0e7e224bfe86830da","unresolved":false,"context_lines":[{"line_number":1082,"context_line":"                    return False, restore_point"},{"line_number":1083,"context_line":""},{"line_number":1084,"context_line":"                return True, restore_point"},{"line_number":1085,"context_line":""},{"line_number":1086,"context_line":"        else:"},{"line_number":1087,"context_line":"            LOG.info(_LI(\"No restore point found for backup\u003d\""},{"line_number":1088,"context_line":"                         \"\u0027%(backup)s\u0027 of volume %(volume)s \""}],"source_content_type":"text/x-python","patch_set":10,"id":"9a41bdd9_5c3cf0e1","line":1085,"in_reply_to":"9a41bdd9_4d6f612b","updated":"2015-07-14 04:00:13.000000000","message":"Done","commit_id":"c4dcad2b5e67297d23560ca10514c711b6d95dcf"}],"cinder/tests/unit/test_backup_ceph.py":[{"author":{"_account_id":2861,"name":"Vincent Hou","email":"shou@us.ibm.com","username":"houshengbo"},"change_message_id":"976f47aaa61b9e35bb3015062bdc56067266a34d","unresolved":false,"context_lines":[{"line_number":812,"context_line":"        # 2. destination volume is not an rbd."},{"line_number":813,"context_line":"        # 3. restore point exists"},{"line_number":814,"context_line":"        with mock.patch.object(self.service, \u0027_rbd_image_exists\u0027) as \\"},{"line_number":815,"context_line":"                mock_rbd_image_exists:"},{"line_number":816,"context_line":"            mock_rbd_image_exists.return_value \u003d (True, backup_base)"},{"line_number":817,"context_line":"            with mock.patch.object(self.service, \u0027_get_restore_point\u0027) as \\"},{"line_number":818,"context_line":"                    mock_get_restore_point:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_0abcea6a","line":815,"updated":"2015-07-07 11:11:42.000000000","message":"I just have one suggestion to avoid the nested mocks.\nYou can try something like this as it applies to all of your tests:\n\nwith mock.patch.object(self.service, \u0027_rbd_image_exists\u0027) as \\\n                   mock_rbd_image_exists,\\\n            mock.patch(self.service, \u0027_get_restore_point\u0027) as \\\n                   mock_get_restore_point...\n        .....\n\nSo multiple mocks will be in the same level instead of several levels.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"84af3f42636e47010268a29107a88f66a286bed2","unresolved":false,"context_lines":[{"line_number":812,"context_line":"        # 2. destination volume is not an rbd."},{"line_number":813,"context_line":"        # 3. restore point exists"},{"line_number":814,"context_line":"        with mock.patch.object(self.service, \u0027_rbd_image_exists\u0027) as \\"},{"line_number":815,"context_line":"                mock_rbd_image_exists:"},{"line_number":816,"context_line":"            mock_rbd_image_exists.return_value \u003d (True, backup_base)"},{"line_number":817,"context_line":"            with mock.patch.object(self.service, \u0027_get_restore_point\u0027) as \\"},{"line_number":818,"context_line":"                    mock_get_restore_point:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_c466ceba","line":815,"in_reply_to":"ba3cc151_0abcea6a","updated":"2015-07-08 02:23:33.000000000","message":"In fact, I wrote this code to keep same style in this file.","commit_id":"7181adea2c4504616c74cff1e4161535fb5d0d69"},{"author":{"_account_id":8846,"name":"Hao Wang","display_name":"Hao Wang","email":"sxmatch1986@gmail.com","username":"sxmatch"},"change_message_id":"1c27bc64055da1366b8be7a7200f6d15af88e228","unresolved":false,"context_lines":[{"line_number":784,"context_line":""},{"line_number":785,"context_line":"        # 1. backup base is not diff-format"},{"line_number":786,"context_line":"        with mock.patch.object(self.service, \u0027_rbd_image_exists\u0027) as \\"},{"line_number":787,"context_line":"                mock_rbd_image_exists:"},{"line_number":788,"context_line":"            mock_rbd_image_exists.return_value \u003d (False, backup_base)"},{"line_number":789,"context_line":"            resp \u003d self.service._diff_restore_allowed(*args_vols_different)"},{"line_number":790,"context_line":"            self.assertEqual(not_allowed, resp)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_62b21208","line":787,"updated":"2015-07-09 02:53:17.000000000","message":"Could we use () to instead of \\?  BTW, I\u0027d like to split this test to some small tests to make code easy to read and debug.","commit_id":"d507ebe61c02a604db6b07309feac0fb4b5f3363"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"7b16f36d1c8b144388fef0122b031cc98d4a1070","unresolved":false,"context_lines":[{"line_number":784,"context_line":""},{"line_number":785,"context_line":"        # 1. backup base is not diff-format"},{"line_number":786,"context_line":"        with mock.patch.object(self.service, \u0027_rbd_image_exists\u0027) as \\"},{"line_number":787,"context_line":"                mock_rbd_image_exists:"},{"line_number":788,"context_line":"            mock_rbd_image_exists.return_value \u003d (False, backup_base)"},{"line_number":789,"context_line":"            resp \u003d self.service._diff_restore_allowed(*args_vols_different)"},{"line_number":790,"context_line":"            self.assertEqual(not_allowed, resp)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_b2be2563","line":787,"in_reply_to":"ba3cc151_62b21208","updated":"2015-07-09 05:23:53.000000000","message":"I agree to split it into several tests.\nBut I keep \\. It seems \\ is a common use.","commit_id":"d507ebe61c02a604db6b07309feac0fb4b5f3363"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a4b165188087d1b8992eff897e30abe659276582","unresolved":false,"context_lines":[{"line_number":784,"context_line":""},{"line_number":785,"context_line":"        # 1. backup base is not diff-format"},{"line_number":786,"context_line":"        with mock.patch.object(self.service, \u0027_rbd_image_exists\u0027) as \\"},{"line_number":787,"context_line":"                mock_rbd_image_exists:"},{"line_number":788,"context_line":"            mock_rbd_image_exists.return_value \u003d (False, backup_base)"},{"line_number":789,"context_line":"            resp \u003d self.service._diff_restore_allowed(*args_vols_different)"},{"line_number":790,"context_line":"            self.assertEqual(not_allowed, resp)"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_2f6e2300","line":787,"in_reply_to":"ba3cc151_b2be2563","updated":"2015-07-09 10:38:31.000000000","message":"It is common use, but something we are trying to move away from, as specified in guidelines - General section, Second bullet point: http://docs.openstack.org/developer/hacking/","commit_id":"d507ebe61c02a604db6b07309feac0fb4b5f3363"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":773,"context_line":""},{"line_number":774,"context_line":"    @common_mocks"},{"line_number":775,"context_line":"    def test_diff_restore_allowed_with_image_not_exists(self):"},{"line_number":776,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when backup not diff-format.\u0027\u0027\u0027"},{"line_number":777,"context_line":"        not_allowed \u003d (False, None)"},{"line_number":778,"context_line":"        backup_base \u003d \u0027backup.base\u0027"},{"line_number":779,"context_line":"        rbd_io \u003d self._get_wrapped_rbd_io(self.service.rbd.Image())"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_01e0d327","line":776,"updated":"2015-07-09 10:39:19.000000000","message":"not allowed","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":773,"context_line":""},{"line_number":774,"context_line":"    @common_mocks"},{"line_number":775,"context_line":"    def test_diff_restore_allowed_with_image_not_exists(self):"},{"line_number":776,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when backup not diff-format.\u0027\u0027\u0027"},{"line_number":777,"context_line":"        not_allowed \u003d (False, None)"},{"line_number":778,"context_line":"        backup_base \u003d \u0027backup.base\u0027"},{"line_number":779,"context_line":"        rbd_io \u003d self._get_wrapped_rbd_io(self.service.rbd.Image())"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_74f61092","line":776,"in_reply_to":"ba3cc151_01e0d327","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":785,"context_line":"            mock_rbd_image_exists.return_value \u003d (False, backup_base)"},{"line_number":786,"context_line":"            resp \u003d self.service._diff_restore_allowed(*args_vols_different)"},{"line_number":787,"context_line":"            self.assertEqual(not_allowed, resp)"},{"line_number":788,"context_line":"            self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":789,"context_line":""},{"line_number":790,"context_line":"    @common_mocks"},{"line_number":791,"context_line":"    def test_diff_restore_allowed_with_no_restore_point(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_a1e247d3","line":788,"updated":"2015-07-09 10:39:19.000000000","message":"You should check what arguments it has been called with using mock_rbd_image_exists.asser_called_once_with( ... )","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":785,"context_line":"            mock_rbd_image_exists.return_value \u003d (False, backup_base)"},{"line_number":786,"context_line":"            resp \u003d self.service._diff_restore_allowed(*args_vols_different)"},{"line_number":787,"context_line":"            self.assertEqual(not_allowed, resp)"},{"line_number":788,"context_line":"            self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":789,"context_line":""},{"line_number":790,"context_line":"    @common_mocks"},{"line_number":791,"context_line":"    def test_diff_restore_allowed_with_no_restore_point(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_746b7020","line":788,"in_reply_to":"ba3cc151_a1e247d3","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":789,"context_line":""},{"line_number":790,"context_line":"    @common_mocks"},{"line_number":791,"context_line":"    def test_diff_restore_allowed_with_no_restore_point(self):"},{"line_number":792,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when no restore point found."},{"line_number":793,"context_line":""},{"line_number":794,"context_line":"        Detail conditions:"},{"line_number":795,"context_line":"          1. backup base is diff-format"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_e1e04f28","line":792,"updated":"2015-07-09 10:39:19.000000000","message":"not allowed","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":789,"context_line":""},{"line_number":790,"context_line":"    @common_mocks"},{"line_number":791,"context_line":"    def test_diff_restore_allowed_with_no_restore_point(self):"},{"line_number":792,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when no restore point found."},{"line_number":793,"context_line":""},{"line_number":794,"context_line":"        Detail conditions:"},{"line_number":795,"context_line":"          1. backup base is diff-format"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_3475e8f9","line":792,"in_reply_to":"ba3cc151_e1e04f28","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":793,"context_line":""},{"line_number":794,"context_line":"        Detail conditions:"},{"line_number":795,"context_line":"          1. backup base is diff-format"},{"line_number":796,"context_line":"          2. restore point does not exists"},{"line_number":797,"context_line":"        \u0027\u0027\u0027"},{"line_number":798,"context_line":"        not_allowed \u003d (False, None)"},{"line_number":799,"context_line":"        backup_base \u003d \u0027backup.base\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_e1160f48","line":796,"updated":"2015-07-09 10:39:19.000000000","message":"s/exists/exist","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":793,"context_line":""},{"line_number":794,"context_line":"        Detail conditions:"},{"line_number":795,"context_line":"          1. backup base is diff-format"},{"line_number":796,"context_line":"          2. restore point does not exists"},{"line_number":797,"context_line":"        \u0027\u0027\u0027"},{"line_number":798,"context_line":"        not_allowed \u003d (False, None)"},{"line_number":799,"context_line":"        backup_base \u003d \u0027backup.base\u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_f45d407d","line":796,"in_reply_to":"ba3cc151_e1160f48","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":811,"context_line":"                args \u003d args_vols_different"},{"line_number":812,"context_line":"                resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":813,"context_line":"                self.assertEqual(not_allowed, resp)"},{"line_number":814,"context_line":"                self.assertTrue(mock_get_restore_point.called)"},{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    @common_mocks"},{"line_number":817,"context_line":"    def test_diff_restore_allowed_with_not_rbd(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_a109e796","line":814,"updated":"2015-07-09 10:39:19.000000000","message":"You should check what arguments it has been called with using mock_get_restore_point.asser_called_once_with( ... )","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":811,"context_line":"                args \u003d args_vols_different"},{"line_number":812,"context_line":"                resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":813,"context_line":"                self.assertEqual(not_allowed, resp)"},{"line_number":814,"context_line":"                self.assertTrue(mock_get_restore_point.called)"},{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    @common_mocks"},{"line_number":817,"context_line":"    def test_diff_restore_allowed_with_not_rbd(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_7452906a","line":814,"in_reply_to":"ba3cc151_a109e796","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    @common_mocks"},{"line_number":817,"context_line":"    def test_diff_restore_allowed_with_not_rbd(self):"},{"line_number":818,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when destination volume is not rbd."},{"line_number":819,"context_line":""},{"line_number":820,"context_line":"        Detail conditions:"},{"line_number":821,"context_line":"          1. backup base is diff-format"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_c153cbe0","line":818,"updated":"2015-07-09 10:39:19.000000000","message":"not allowed","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":815,"context_line":""},{"line_number":816,"context_line":"    @common_mocks"},{"line_number":817,"context_line":"    def test_diff_restore_allowed_with_not_rbd(self):"},{"line_number":818,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when destination volume is not rbd."},{"line_number":819,"context_line":""},{"line_number":820,"context_line":"        Detail conditions:"},{"line_number":821,"context_line":"          1. backup base is diff-format"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_944f14be","line":818,"in_reply_to":"ba3cc151_c153cbe0","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":840,"context_line":"                    args \u003d args_vols_different"},{"line_number":841,"context_line":"                    resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":842,"context_line":"                    self.assertEqual((False, restore_point), resp)"},{"line_number":843,"context_line":"                    self.assertTrue(mock_get_restore_point.called)"},{"line_number":844,"context_line":"                    self.assertTrue(mock_file_is_rbd.called)"},{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_41333b62","line":843,"updated":"2015-07-09 10:39:19.000000000","message":"Check arguments that have been used in the call","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"cc136f3499a46f72b00c34f78d3ae0c9af3d2c15","unresolved":false,"context_lines":[{"line_number":840,"context_line":"                    args \u003d args_vols_different"},{"line_number":841,"context_line":"                    resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":842,"context_line":"                    self.assertEqual((False, restore_point), resp)"},{"line_number":843,"context_line":"                    self.assertTrue(mock_get_restore_point.called)"},{"line_number":844,"context_line":"                    self.assertTrue(mock_file_is_rbd.called)"},{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_4c325cf4","line":843,"in_reply_to":"ba3cc151_14f1c46a","updated":"2015-07-10 08:39:13.000000000","message":"The problem with not checking the arguments is that if at some point some of those tests are removed then we would no longer be testing the calls.\n\nBut I can understand it if you don\u0027t want to check calls that you consider covered by other tests, but for those that you decide to check, you should do it right with all the arguments, and not just with \"called\" method.","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"24d1d6c701bfaa23ff8d8a286fe58b665836cda0","unresolved":false,"context_lines":[{"line_number":840,"context_line":"                    args \u003d args_vols_different"},{"line_number":841,"context_line":"                    resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":842,"context_line":"                    self.assertEqual((False, restore_point), resp)"},{"line_number":843,"context_line":"                    self.assertTrue(mock_get_restore_point.called)"},{"line_number":844,"context_line":"                    self.assertTrue(mock_file_is_rbd.called)"},{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_3bfdc908","line":843,"in_reply_to":"ba3cc151_18a80bdb","updated":"2015-07-10 10:12:24.000000000","message":"I can understand why you think it\u0027s duplicated, but in my experience there have been quite a number of bugs that are related to argument passing (wrong arguments, wrong order or wrong number of arguments) in the past.  So just testing that the method is being called is usually insufficient and only gives us a false sense of security.  But I know for a fact that there are many people that disagree with me, so it\u0027s not a big deal, that\u0027s why I didn\u0027t downvote the patch.  :-)","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":840,"context_line":"                    args \u003d args_vols_different"},{"line_number":841,"context_line":"                    resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":842,"context_line":"                    self.assertEqual((False, restore_point), resp)"},{"line_number":843,"context_line":"                    self.assertTrue(mock_get_restore_point.called)"},{"line_number":844,"context_line":"                    self.assertTrue(mock_file_is_rbd.called)"},{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_14f1c46a","line":843,"in_reply_to":"ba3cc151_41333b62","updated":"2015-07-10 02:24:56.000000000","message":"Hi Gorka, I would not like to check mock_get_restore_point here as I check its arguments in former tests.\nI would like to check arguments for every new called functions in every test. For example, once I check mock_get_restore_point.asser_called_once_with in last test, here I just check mock_get_restore_point.called.","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"3cd277036aa916bec3444e4cfdebc9af8cc81eed","unresolved":false,"context_lines":[{"line_number":840,"context_line":"                    args \u003d args_vols_different"},{"line_number":841,"context_line":"                    resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":842,"context_line":"                    self.assertEqual((False, restore_point), resp)"},{"line_number":843,"context_line":"                    self.assertTrue(mock_get_restore_point.called)"},{"line_number":844,"context_line":"                    self.assertTrue(mock_file_is_rbd.called)"},{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_18a80bdb","line":843,"in_reply_to":"ba3cc151_4c325cf4","updated":"2015-07-10 09:51:11.000000000","message":"Sorry, Gorka. I don\u0027t agree with you here. :)\nBased on your idea, it is best to check all the arguments instead of  every checking called method, Or else there is risks to lose tests if some tests are removed. \nBut I still think it is too duplicated.","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":841,"context_line":"                    resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":842,"context_line":"                    self.assertEqual((False, restore_point), resp)"},{"line_number":843,"context_line":"                    self.assertTrue(mock_get_restore_point.called)"},{"line_number":844,"context_line":"                    self.assertTrue(mock_file_is_rbd.called)"},{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"},{"line_number":847,"context_line":"    def test_diff_restore_allowed_with_same_volume(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_014213ae","line":844,"updated":"2015-07-09 10:39:19.000000000","message":"Same here","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":841,"context_line":"                    resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":842,"context_line":"                    self.assertEqual((False, restore_point), resp)"},{"line_number":843,"context_line":"                    self.assertTrue(mock_get_restore_point.called)"},{"line_number":844,"context_line":"                    self.assertTrue(mock_file_is_rbd.called)"},{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"},{"line_number":847,"context_line":"    def test_diff_restore_allowed_with_same_volume(self):"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_b4e578a5","line":844,"in_reply_to":"ba3cc151_014213ae","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"},{"line_number":847,"context_line":"    def test_diff_restore_allowed_with_same_volume(self):"},{"line_number":848,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when volumes are same."},{"line_number":849,"context_line":""},{"line_number":850,"context_line":"        Detail conditions:"},{"line_number":851,"context_line":"          1. backup base is diff-format"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_21707771","line":848,"updated":"2015-07-09 10:39:19.000000000","message":"not allowed","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":845,"context_line":""},{"line_number":846,"context_line":"    @common_mocks"},{"line_number":847,"context_line":"    def test_diff_restore_allowed_with_same_volume(self):"},{"line_number":848,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when volumes are same."},{"line_number":849,"context_line":""},{"line_number":850,"context_line":"        Detail conditions:"},{"line_number":851,"context_line":"          1. backup base is diff-format"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_d4ea7c74","line":848,"in_reply_to":"ba3cc151_21707771","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d59bdf521cb666257cf74a0599a59a3ba4c959f5","unresolved":false,"context_lines":[{"line_number":878,"context_line":""},{"line_number":879,"context_line":"    @common_mocks"},{"line_number":880,"context_line":"    def test_diff_restore_allowed_with_has_extents(self):"},{"line_number":881,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when destination volume has data."},{"line_number":882,"context_line":""},{"line_number":883,"context_line":"        Detail conditions:"},{"line_number":884,"context_line":"          1. backup base is diff-format"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_810703c9","line":881,"updated":"2015-07-09 10:39:19.000000000","message":"not allowed","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"841d1008ff397f5d313086059fdcb76495dadd33","unresolved":false,"context_lines":[{"line_number":878,"context_line":""},{"line_number":879,"context_line":"    @common_mocks"},{"line_number":880,"context_line":"    def test_diff_restore_allowed_with_has_extents(self):"},{"line_number":881,"context_line":"        \u0027\u0027\u0027Test diff restore allowed when destination volume has data."},{"line_number":882,"context_line":""},{"line_number":883,"context_line":"        Detail conditions:"},{"line_number":884,"context_line":"          1. backup base is diff-format"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_34fac844","line":881,"in_reply_to":"ba3cc151_810703c9","updated":"2015-07-10 02:24:56.000000000","message":"Done","commit_id":"cba072624720029e2dd80899b0b0dcb1c1e3f2a3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"1a95d6bce04f662934dd4488a26d9b753c5548ed","unresolved":false,"context_lines":[{"line_number":817,"context_line":"                resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":818,"context_line":""},{"line_number":819,"context_line":"                self.assertEqual(not_allowed, resp)"},{"line_number":820,"context_line":"                self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":821,"context_line":"                mock_get_restore_point.assert_called_once_with("},{"line_number":822,"context_line":"                    backup_base,"},{"line_number":823,"context_line":"                    self.backup[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":9,"id":"ba3cc151_4c601cd0","line":820,"updated":"2015-07-10 08:39:34.000000000","message":"As commented on line 843 of patch 7, if you are going to check that the method is being called, you should check that it is called with the right arguments.","commit_id":"16039de7836262b3c82a31202267ea4bdcbc8cd6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"7793fcf80659325fed21da7e27640b5b3ce8538f","unresolved":false,"context_lines":[{"line_number":817,"context_line":"                resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":818,"context_line":""},{"line_number":819,"context_line":"                self.assertEqual(not_allowed, resp)"},{"line_number":820,"context_line":"                self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":821,"context_line":"                mock_get_restore_point.assert_called_once_with("},{"line_number":822,"context_line":"                    backup_base,"},{"line_number":823,"context_line":"                    self.backup[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":9,"id":"ba3cc151_58d3f35f","line":820,"in_reply_to":"ba3cc151_4c601cd0","updated":"2015-07-10 09:52:16.000000000","message":"Please see my response in line 843 of patch 7.","commit_id":"16039de7836262b3c82a31202267ea4bdcbc8cd6"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"1a95d6bce04f662934dd4488a26d9b753c5548ed","unresolved":false,"context_lines":[{"line_number":852,"context_line":""},{"line_number":853,"context_line":"                    self.assertEqual((False, restore_point), resp)"},{"line_number":854,"context_line":"                    self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":855,"context_line":"                    self.assertTrue(mock_get_restore_point.called)"},{"line_number":856,"context_line":"                    mock_file_is_rbd.assert_called_once_with("},{"line_number":857,"context_line":"                        rbd_io)"},{"line_number":858,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"ba3cc151_0c99b4d0","line":855,"updated":"2015-07-10 08:39:34.000000000","message":"Same here","commit_id":"16039de7836262b3c82a31202267ea4bdcbc8cd6"},{"author":{"_account_id":2861,"name":"Vincent Hou","email":"shou@us.ibm.com","username":"houshengbo"},"change_message_id":"1a8f3fa5710c78e6cf5153baacd5e680346b117d","unresolved":false,"context_lines":[{"line_number":922,"context_line":"                        args \u003d args_vols_different"},{"line_number":923,"context_line":"                        resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":924,"context_line":""},{"line_number":925,"context_line":"                        self.assertEqual(resp, (False, restore_point))"},{"line_number":926,"context_line":"                        self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":927,"context_line":"                        self.assertTrue(mock_get_restore_point.called)"},{"line_number":928,"context_line":"                        self.assertTrue(mock_file_is_rbd.called)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9a41bdd9_10ae8485","line":925,"updated":"2015-07-13 07:52:36.000000000","message":"Should be self.assertEqual((False, restore_point), resp).","commit_id":"16039de7836262b3c82a31202267ea4bdcbc8cd6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"c02381ced0c534098b218dcc80ccded2e98a75b8","unresolved":false,"context_lines":[{"line_number":922,"context_line":"                        args \u003d args_vols_different"},{"line_number":923,"context_line":"                        resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":924,"context_line":""},{"line_number":925,"context_line":"                        self.assertEqual(resp, (False, restore_point))"},{"line_number":926,"context_line":"                        self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":927,"context_line":"                        self.assertTrue(mock_get_restore_point.called)"},{"line_number":928,"context_line":"                        self.assertTrue(mock_file_is_rbd.called)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9a41bdd9_7cb6c78b","line":925,"in_reply_to":"9a41bdd9_10ae8485","updated":"2015-07-13 09:28:43.000000000","message":"Done","commit_id":"16039de7836262b3c82a31202267ea4bdcbc8cd6"},{"author":{"_account_id":2861,"name":"Vincent Hou","email":"shou@us.ibm.com","username":"houshengbo"},"change_message_id":"1a8f3fa5710c78e6cf5153baacd5e680346b117d","unresolved":false,"context_lines":[{"line_number":962,"context_line":"                        args \u003d args_vols_different"},{"line_number":963,"context_line":"                        resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":964,"context_line":""},{"line_number":965,"context_line":"                        self.assertEqual(resp, (True, restore_point))"},{"line_number":966,"context_line":"                        self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":967,"context_line":"                        self.assertTrue(mock_get_restore_point.called)"},{"line_number":968,"context_line":"                        self.assertTrue(mock_file_is_rbd.called)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9a41bdd9_b087980e","line":965,"updated":"2015-07-13 07:52:36.000000000","message":"Should be self.assertEqual((True, restore_point), resp)","commit_id":"16039de7836262b3c82a31202267ea4bdcbc8cd6"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"c02381ced0c534098b218dcc80ccded2e98a75b8","unresolved":false,"context_lines":[{"line_number":962,"context_line":"                        args \u003d args_vols_different"},{"line_number":963,"context_line":"                        resp \u003d self.service._diff_restore_allowed(*args)"},{"line_number":964,"context_line":""},{"line_number":965,"context_line":"                        self.assertEqual(resp, (True, restore_point))"},{"line_number":966,"context_line":"                        self.assertTrue(mock_rbd_image_exists.called)"},{"line_number":967,"context_line":"                        self.assertTrue(mock_get_restore_point.called)"},{"line_number":968,"context_line":"                        self.assertTrue(mock_file_is_rbd.called)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9a41bdd9_c774e09d","line":965,"in_reply_to":"9a41bdd9_b087980e","updated":"2015-07-13 09:28:43.000000000","message":"Done","commit_id":"16039de7836262b3c82a31202267ea4bdcbc8cd6"}]}