)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":5280,"name":"Nicolas Simonds","email":"nic@metacloud.com","username":"nicolas.simonds"},"change_message_id":"2b3b1395ff3a7ad920ea37806250504d15e11875","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Srinivas Sakhamuri \u003csrini.openstack@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-11-05 11:34:52 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Image uploads to the filesystem driver are not fully atomic"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This change fixes the issues that occur with upload failures and"},{"line_number":10,"context_line":"partial files that result because of such failures by making"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"fa80f949_af5efc0f","line":7,"updated":"2015-11-05 20:13:14.000000000","message":"The commit message should explain what the code does after the merge, not how the code is currently broken.  e.g., \"Make image uploads to the filesystem driver fully atomic\" would be a better title.","commit_id":"1a5d5564e688a7ea4cc37d227cb8a91fbf1891cb"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"770d218a6f712c6c8f5e76bb7b6b8888d6cda520","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This change makes the filesystem uploads appear atomic by making"},{"line_number":10,"context_line":"use of temp file for uploads before moving to final destination."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Closes-bug: 1297451"},{"line_number":13,"context_line":"Change-Id: Ie03837f5ff45aebd1fb61160da42fc03f92268e7"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"ba8a016a_d74cb59f","line":11,"updated":"2015-11-23 10:16:18.000000000","message":"Need DocImpact flag","commit_id":"bc9eaa0cdd865b956d7f4b7d086d18cd2876b2be"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"85676da015c97cf00e0a9b2156c362c86ab95b49","unresolved":false,"context_lines":[{"line_number":9,"context_line":"This change makes the filesystem uploads appear atomic by making"},{"line_number":10,"context_line":"use of temp file for uploads before moving to final destination"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"DocImpact"},{"line_number":13,"context_line":"Closes-bug: 1297451"},{"line_number":14,"context_line":"Change-Id: Ie03837f5ff45aebd1fb61160da42fc03f92268e7"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":16,"id":"9a30ddce_1bd14bfc","line":12,"range":{"start_line":12,"start_character":0,"end_line":12,"end_character":9},"updated":"2017-03-06 23:52:53.000000000","message":"I believe it is no longer meaningful to have DocImpact in the commit message like this. If documentation changes are called for they should be briefly summarized at least within the commit message.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"}],"glance_store/_drivers/filesystem.py":[{"author":{"_account_id":5280,"name":"Nicolas Simonds","email":"nic@metacloud.com","username":"nicolas.simonds"},"change_message_id":"2b3b1395ff3a7ad920ea37806250504d15e11875","unresolved":false,"context_lines":[{"line_number":630,"context_line":"            try:"},{"line_number":631,"context_line":"                LOG.debug(_(\"Moving %(temppath)s into place as %(filepath)s\")"},{"line_number":632,"context_line":"                          % dict(temppath\u003dtemppath, filepath\u003dfilepath))"},{"line_number":633,"context_line":"                os.rename(temppath, filepath)"},{"line_number":634,"context_line":"            except Exception:"},{"line_number":635,"context_line":"                self._delete_partial(temppath, image_id)"},{"line_number":636,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"fa80f949_af303ce9","line":633,"updated":"2015-11-05 20:13:14.000000000","message":"If the temppath and the filepath are on different filesystems, this is going to fail, and the exception handler is going to clean things up without reporting any errors.\n\nTo get around this, you could use shutil.move(), but then it would no longer necessarily be atomic.  Code to verify that the temppath and filepath are on the same filesystem should be added to avoid corner-case bugs.","commit_id":"1a5d5564e688a7ea4cc37d227cb8a91fbf1891cb"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"5b86a07d8c8d3f9b78c8c65baf726895143e0d80","unresolved":false,"context_lines":[{"line_number":630,"context_line":"            try:"},{"line_number":631,"context_line":"                LOG.debug(_(\"Moving %(temppath)s into place as %(filepath)s\")"},{"line_number":632,"context_line":"                          % dict(temppath\u003dtemppath, filepath\u003dfilepath))"},{"line_number":633,"context_line":"                os.rename(temppath, filepath)"},{"line_number":634,"context_line":"            except Exception:"},{"line_number":635,"context_line":"                self._delete_partial(temppath, image_id)"},{"line_number":636,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"fa80f949_eac0727e","line":633,"in_reply_to":"fa80f949_af303ce9","updated":"2015-11-05 20:48:37.000000000","message":"I think in the case of different file systems shall we just not use this mechanism at all? since we cannot guarantee atomicity any way and also performance may suffer due to double copy of data.\n\nAnother option I can think of is use a relative directory, without having an option to configure a temp directory. I don\u0027t see a value having a config option for the temp directory especially with user potentially using a non-ideal value like the above.","commit_id":"1a5d5564e688a7ea4cc37d227cb8a91fbf1891cb"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"770d218a6f712c6c8f5e76bb7b6b8888d6cda520","unresolved":false,"context_lines":[{"line_number":233,"context_line":"                    self._check_write_permission(datadir)"},{"line_number":234,"context_line":"                    self._set_exec_permission(datadir)"},{"line_number":235,"context_line":"                else:"},{"line_number":236,"context_line":"                    msg \u003d _(\"Directory to write image files does not exist \""},{"line_number":237,"context_line":"                            \"(%s). Creating.\") % datadir"},{"line_number":238,"context_line":"                    LOG.info(msg)"},{"line_number":239,"context_line":"                    try:"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba8a016a_5723e555","line":236,"updated":"2015-11-23 10:16:18.000000000","message":"this should be translated using _LI","commit_id":"bc9eaa0cdd865b956d7f4b7d086d18cd2876b2be"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"770d218a6f712c6c8f5e76bb7b6b8888d6cda520","unresolved":false,"context_lines":[{"line_number":605,"context_line":"            # If filepath and temppath are in different filesystem"},{"line_number":606,"context_line":"            # os.rename fails and we don\u0027t achieve atomicity in"},{"line_number":607,"context_line":"            # uploads, log a warning and go with normal upload"},{"line_number":608,"context_line":"            reason \u003d (_(\"filesystem_store_datadir(%(filepath)s) and \""},{"line_number":609,"context_line":"                        \"filesystem_store_tempdir(%(temppath)s) cannot \""},{"line_number":610,"context_line":"                        \"be located on different filesystems\") %"},{"line_number":611,"context_line":"                      dict(filepath\u003dfilepath, temppath\u003dtemppath))"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba8a016a_f7cc5936","line":608,"updated":"2015-11-23 10:16:18.000000000","message":"Need to translate with _LW and need white space after filesystem_store_datadir","commit_id":"bc9eaa0cdd865b956d7f4b7d086d18cd2876b2be"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"770d218a6f712c6c8f5e76bb7b6b8888d6cda520","unresolved":false,"context_lines":[{"line_number":606,"context_line":"            # os.rename fails and we don\u0027t achieve atomicity in"},{"line_number":607,"context_line":"            # uploads, log a warning and go with normal upload"},{"line_number":608,"context_line":"            reason \u003d (_(\"filesystem_store_datadir(%(filepath)s) and \""},{"line_number":609,"context_line":"                        \"filesystem_store_tempdir(%(temppath)s) cannot \""},{"line_number":610,"context_line":"                        \"be located on different filesystems\") %"},{"line_number":611,"context_line":"                      dict(filepath\u003dfilepath, temppath\u003dtemppath))"},{"line_number":612,"context_line":"            LOG.warn(reason)"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba8a016a_eff2fa05","line":609,"updated":"2015-11-23 10:16:18.000000000","message":"need white space after filesystem_store_tempdir","commit_id":"bc9eaa0cdd865b956d7f4b7d086d18cd2876b2be"},{"author":{"_account_id":17976,"name":"Drew Varner","email":"avarner@linux.vnet.ibm.com","username":"avarner"},"change_message_id":"3ead262f8b70a4ef66356d6fe6fe5c60d0023666","unresolved":false,"context_lines":[{"line_number":53,"context_line":"    cfg.MultiStrOpt(\u0027filesystem_store_datadirs\u0027,"},{"line_number":54,"context_line":"                    help\u003d_(\"List of directories and its priorities to which \""},{"line_number":55,"context_line":"                           \"the Filesystem backend store writes images.\")),"},{"line_number":56,"context_line":"    cfg.StrOpt(\u0027filesystem_store_tempdir\u0027, default\u003d\u0027\u0027,"},{"line_number":57,"context_line":"               help\u003d_(\u0027Temporary directory used by the Filesystem backend, \u0027"},{"line_number":58,"context_line":"                      \u0027relative to the filesystem_store_datadir or \u0027"},{"line_number":59,"context_line":"                      \u0027filesystem_store_datadirs\u0027)),"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a8ffd7b_744fdd92","line":56,"updated":"2015-11-24 22:50:28.000000000","message":"I\u0027d like to bring up Zhi Yan Liu\u0027s question[1] from 2014-03-26 - why is the temporary directory specified with a relative path, instead of an absolute path?\n\nNicolas Simonds answered that the temporary directory is intended to be a subdirectory or sibling directory of the datadir.\n\nI think in the case of a sibling directory, an absolute path is much more intuitive. However, in the case of multiple datadirs on different filesystems, we would have to implement corresponding tempdirs for each one.\n\n[1] https://review.openstack.org/#/c/82899/3/glance/store/filesystem.py","commit_id":"0a01a27b5fc1da7aebdd53f92211b24f1f178262"},{"author":{"_account_id":17976,"name":"Drew Varner","email":"avarner@linux.vnet.ibm.com","username":"avarner"},"change_message_id":"86cd6c60a0016f75492e420eca844489e15dd974","unresolved":false,"context_lines":[{"line_number":53,"context_line":"    cfg.MultiStrOpt(\u0027filesystem_store_datadirs\u0027,"},{"line_number":54,"context_line":"                    help\u003d_(\"List of directories and its priorities to which \""},{"line_number":55,"context_line":"                           \"the Filesystem backend store writes images.\")),"},{"line_number":56,"context_line":"    cfg.StrOpt(\u0027filesystem_store_tempdir\u0027, default\u003d\u0027\u0027,"},{"line_number":57,"context_line":"               help\u003d_(\u0027Temporary directory used by the Filesystem backend, \u0027"},{"line_number":58,"context_line":"                      \u0027relative to the filesystem_store_datadir or \u0027"},{"line_number":59,"context_line":"                      \u0027filesystem_store_datadirs\u0027)),"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a740942_d5dff5d3","line":56,"in_reply_to":"9a8ffd7b_4a88f222","updated":"2015-12-08 20:13:26.000000000","message":"Naming it \"filesystem_store_relative_tempdir\" might help too.","commit_id":"0a01a27b5fc1da7aebdd53f92211b24f1f178262"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"c54e5b0420f02cd4cdb4e26e81cfb9905acb7e13","unresolved":false,"context_lines":[{"line_number":53,"context_line":"    cfg.MultiStrOpt(\u0027filesystem_store_datadirs\u0027,"},{"line_number":54,"context_line":"                    help\u003d_(\"List of directories and its priorities to which \""},{"line_number":55,"context_line":"                           \"the Filesystem backend store writes images.\")),"},{"line_number":56,"context_line":"    cfg.StrOpt(\u0027filesystem_store_tempdir\u0027, default\u003d\u0027\u0027,"},{"line_number":57,"context_line":"               help\u003d_(\u0027Temporary directory used by the Filesystem backend, \u0027"},{"line_number":58,"context_line":"                      \u0027relative to the filesystem_store_datadir or \u0027"},{"line_number":59,"context_line":"                      \u0027filesystem_store_datadirs\u0027)),"}],"source_content_type":"text/x-python","patch_set":7,"id":"9a8ffd7b_4a88f222","line":56,"in_reply_to":"9a8ffd7b_744fdd92","updated":"2015-11-24 23:24:02.000000000","message":"The reason for keeping it relative is to force operators to think that data dir and temp dir are supposed to be in same file system. Otherwise the atomicity that we want to achieve will not be possible since the move at the end of upload will actual be a 2nd copy of the image file.\n\nThere is also check in the \u0027add\u0027 to make sure both dirs are from same filesystem otherwise it will just do a regular copy.","commit_id":"0a01a27b5fc1da7aebdd53f92211b24f1f178262"},{"author":{"_account_id":2537,"name":"Nikhil Komawar","email":"nik.komawar@gmail.com","username":"nikhil-komawar"},"change_message_id":"800bcd46cbcb0eb223f94dd8759dccf279982e2e","unresolved":false,"context_lines":[{"line_number":654,"context_line":"                          % dict(temppath\u003dtemppath, filepath\u003dfilepath))"},{"line_number":655,"context_line":"                os.rename(temppath, filepath)"},{"line_number":656,"context_line":"            except Exception:"},{"line_number":657,"context_line":"                self._delete_partial(temppath, image_id)"},{"line_number":658,"context_line":""},{"line_number":659,"context_line":"        checksum_hex \u003d checksum.hexdigest()"},{"line_number":660,"context_line":"        metadata \u003d self._get_metadata(filepath)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9aed3d3a_a8557ac7","line":657,"range":{"start_line":657,"start_character":21,"end_line":657,"end_character":36},"updated":"2016-02-25 18:55:30.000000000","message":"this looks very risky removal without any indication/re-raise!","commit_id":"030f247eec76072346bf7b287c9704b4f5a95eb3"},{"author":{"_account_id":5280,"name":"Nicolas Simonds","email":"nic@metacloud.com","username":"nicolas.simonds"},"change_message_id":"69fc8dcf99358c0b1fad0f4307e456a4a4ec7f61","unresolved":false,"context_lines":[{"line_number":654,"context_line":"                          % dict(temppath\u003dtemppath, filepath\u003dfilepath))"},{"line_number":655,"context_line":"                os.rename(temppath, filepath)"},{"line_number":656,"context_line":"            except Exception:"},{"line_number":657,"context_line":"                self._delete_partial(temppath, image_id)"},{"line_number":658,"context_line":""},{"line_number":659,"context_line":"        checksum_hex \u003d checksum.hexdigest()"},{"line_number":660,"context_line":"        metadata \u003d self._get_metadata(filepath)"}],"source_content_type":"text/x-python","patch_set":9,"id":"9aed3d3a_c87d5ee9","line":657,"range":{"start_line":657,"start_character":21,"end_line":657,"end_character":36},"in_reply_to":"9aed3d3a_a8557ac7","updated":"2016-02-25 19:03:14.000000000","message":"Agreed.  Effectively aborting the upload without any indication that anything went wrong is confusing behavior at best.","commit_id":"030f247eec76072346bf7b287c9704b4f5a95eb3"},{"author":{"_account_id":22448,"name":"Alexander Bashmakov","email":"abashmak@yahoo.com","username":"abashmak"},"change_message_id":"5215785216a6cc3b84522afb0ee01ad1e34de5c0","unresolved":false,"context_lines":[{"line_number":107,"context_line":""},{"line_number":108,"context_line":"\"\"\")),"},{"line_number":109,"context_line":"    cfg.StrOpt(\u0027filesystem_store_tempdir\u0027, default\u003d\u0027\u0027,"},{"line_number":110,"context_line":"               help\u003d_(\u0027Temporary directory used by the Filesystem backend, \u0027"},{"line_number":111,"context_line":"                      \u0027relative to the filesystem_store_datadir or \u0027"},{"line_number":112,"context_line":"                      \u0027filesystem_store_datadirs\u0027)),"},{"line_number":113,"context_line":"    cfg.StrOpt(\u0027filesystem_store_metadata_file\u0027,"},{"line_number":114,"context_line":"               help\u003d_(\"\"\""},{"line_number":115,"context_line":"Filesystem store metadata file."}],"source_content_type":"text/x-python","patch_set":12,"id":"9a89bdaa_cdda1e4b","line":112,"range":{"start_line":110,"start_character":0,"end_line":112,"end_character":52},"updated":"2016-09-12 22:53:39.000000000","message":"In keeping with our recently merged improved config options initiative, please update the help text appropriately for this new option. See https://github.com/openstack/glance_store/blob/master/glance_store/backend.py#L37 for an example","commit_id":"c99a8cfee80c1f62449194f868b9005e9e96ac53"},{"author":{"_account_id":22448,"name":"Alexander Bashmakov","email":"abashmak@yahoo.com","username":"abashmak"},"change_message_id":"d87cc82c629193f3242623022368639bffcbb1af","unresolved":false,"context_lines":[{"line_number":321,"context_line":"        \"\"\""},{"line_number":322,"context_line":"        for path in directory_paths:"},{"line_number":323,"context_line":"            tempdir \u003d os.path.normpath(os.path.join("},{"line_number":324,"context_line":"                path, self.conf.glance_store.filesystem_store_tempdir))"},{"line_number":325,"context_line":""},{"line_number":326,"context_line":"            for datadir in set([path, tempdir]):"},{"line_number":327,"context_line":"                if os.path.exists(datadir):"}],"source_content_type":"text/x-python","patch_set":13,"id":"7a8ec9b2_55ec608f","line":324,"range":{"start_line":324,"start_character":45,"end_line":324,"end_character":69},"updated":"2016-09-19 16:54:37.000000000","message":"What happens when this is not set? Will a BadStoreConfiguration exception be raised?","commit_id":"961a6a28a6a499420a591fdbccf6645204c55a8a"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"393fec07a0fe5763fb0390a0701e470d1beebfb5","unresolved":false,"context_lines":[{"line_number":321,"context_line":"        \"\"\""},{"line_number":322,"context_line":"        for path in directory_paths:"},{"line_number":323,"context_line":"            tempdir \u003d os.path.normpath(os.path.join("},{"line_number":324,"context_line":"                path, self.conf.glance_store.filesystem_store_tempdir))"},{"line_number":325,"context_line":""},{"line_number":326,"context_line":"            for datadir in set([path, tempdir]):"},{"line_number":327,"context_line":"                if os.path.exists(datadir):"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa6399be_d9af9d92","line":324,"range":{"start_line":324,"start_character":45,"end_line":324,"end_character":69},"in_reply_to":"7a8ec9b2_55ec608f","updated":"2016-10-18 15:55:47.000000000","message":"if the tempdir is not specified we don\u0027t use tempdir for intermediate storage, it directly stores in final path. We will only be creating the directory path for image storage , no temp dir gets get\u0027s created","commit_id":"961a6a28a6a499420a591fdbccf6645204c55a8a"},{"author":{"_account_id":22448,"name":"Alexander Bashmakov","email":"abashmak@yahoo.com","username":"abashmak"},"change_message_id":"0baf46bbcf8e2c5c5e2f6563beed531a9665eb97","unresolved":false,"context_lines":[{"line_number":321,"context_line":"        \"\"\""},{"line_number":322,"context_line":"        for path in directory_paths:"},{"line_number":323,"context_line":"            tempdir \u003d os.path.normpath(os.path.join("},{"line_number":324,"context_line":"                path, self.conf.glance_store.filesystem_store_tempdir))"},{"line_number":325,"context_line":""},{"line_number":326,"context_line":"            for datadir in set([path, tempdir]):"},{"line_number":327,"context_line":"                if os.path.exists(datadir):"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa6399be_eb5d59ed","line":324,"range":{"start_line":324,"start_character":45,"end_line":324,"end_character":69},"in_reply_to":"fa6399be_d9af9d92","updated":"2016-10-18 22:01:32.000000000","message":"Here it seems that filesytem_store_tempdir will always be used, regardless if it\u0027s specified or not. In the case it\u0027s not specified, `tempdir` becomes equal to `path` (lines 323-324). But then it gets optimized out by the set call in the for-loop on line 326. So I\u0027m convinced this works as expected.","commit_id":"961a6a28a6a499420a591fdbccf6645204c55a8a"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"de78e646c0c296b761f08985b781ffbea6135533","unresolved":false,"context_lines":[{"line_number":112,"context_line":""},{"line_number":113,"context_line":"A temporary directory that is relative to the ``filesystem_store_datadir``"},{"line_number":114,"context_line":"or ``filesystem_store_datadirs``. File system driver uses this as a temporary"},{"line_number":115,"context_line":"place to store the uploaded images moves it to final directory once the upload"},{"line_number":116,"context_line":"is complete."},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"Possible values:"}],"source_content_type":"text/x-python","patch_set":15,"id":"fa31d9ce_5ff2cf9b","line":115,"range":{"start_line":115,"start_character":34,"end_line":115,"end_character":35},"updated":"2017-02-10 02:46:02.000000000","message":"s//and /","commit_id":"23d6d9c04cf3fffda65e37961e1ed3201156ada8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"de78e646c0c296b761f08985b781ffbea6135533","unresolved":false,"context_lines":[{"line_number":699,"context_line":"        if os.path.exists(filepath):"},{"line_number":700,"context_line":"            raise exceptions.Duplicate(image\u003dfilepath)"},{"line_number":701,"context_line":""},{"line_number":702,"context_line":"        if os.stat(os.path.dirname(filepath)).st_dev !\u003d \\"},{"line_number":703,"context_line":"                os.stat(os.path.dirname(temppath)).st_dev:"},{"line_number":704,"context_line":"            # If filepath and temppath are in different filesystem"},{"line_number":705,"context_line":"            # os.rename fails and we don\u0027t achieve atomicity in"},{"line_number":706,"context_line":"            # uploads, log a warning and go with normal upload"}],"source_content_type":"text/x-python","patch_set":15,"id":"fa31d9ce_0773a9b2","line":703,"range":{"start_line":702,"start_character":8,"end_line":703,"end_character":58},"updated":"2017-02-10 02:46:02.000000000","message":"Why not use os.statvfs(path)?","commit_id":"23d6d9c04cf3fffda65e37961e1ed3201156ada8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"672f7d7b9f526bbfe2b2dd2f30780b9f3735f666","unresolved":false,"context_lines":[{"line_number":699,"context_line":"        if os.path.exists(filepath):"},{"line_number":700,"context_line":"            raise exceptions.Duplicate(image\u003dfilepath)"},{"line_number":701,"context_line":""},{"line_number":702,"context_line":"        if os.stat(os.path.dirname(filepath)).st_dev !\u003d \\"},{"line_number":703,"context_line":"                os.stat(os.path.dirname(temppath)).st_dev:"},{"line_number":704,"context_line":"            # If filepath and temppath are in different filesystem"},{"line_number":705,"context_line":"            # os.rename fails and we don\u0027t achieve atomicity in"},{"line_number":706,"context_line":"            # uploads, log a warning and go with normal upload"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba2be162_5923c405","line":703,"range":{"start_line":702,"start_character":8,"end_line":703,"end_character":58},"in_reply_to":"ba2be162_4fc77e45","updated":"2017-02-28 18:10:33.000000000","message":"Srinivas, I was just wondering if we could use stavfs parameters for our verification. But I guess I missed noticing that we do not get a major/minor device number out of it.\nSo, I am okay with the current method of extracting the device number.\nThanks.","commit_id":"23d6d9c04cf3fffda65e37961e1ed3201156ada8"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"ff935e82a19a3c7e72b73063e9269577fe3fed83","unresolved":false,"context_lines":[{"line_number":699,"context_line":"        if os.path.exists(filepath):"},{"line_number":700,"context_line":"            raise exceptions.Duplicate(image\u003dfilepath)"},{"line_number":701,"context_line":""},{"line_number":702,"context_line":"        if os.stat(os.path.dirname(filepath)).st_dev !\u003d \\"},{"line_number":703,"context_line":"                os.stat(os.path.dirname(temppath)).st_dev:"},{"line_number":704,"context_line":"            # If filepath and temppath are in different filesystem"},{"line_number":705,"context_line":"            # os.rename fails and we don\u0027t achieve atomicity in"},{"line_number":706,"context_line":"            # uploads, log a warning and go with normal upload"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba2be162_4fc77e45","line":703,"range":{"start_line":702,"start_character":8,"end_line":703,"end_character":58},"in_reply_to":"fa31d9ce_0773a9b2","updated":"2017-02-28 15:57:11.000000000","message":"Not sure that will help what we are trying to do, I tried following example on Centos\n\n\u003e\u003e\u003e import os\n\u003e\u003e\u003e st \u003d os.stat(\"/home/vagrant/.ssh/authorized_keys\")\n\u003e\u003e\u003e st\nposix.stat_result(st_mode\u003d33152, st_ino\u003d67147326, st_dev\u003d64768L, st_nlink\u003d1, st_uid\u003d1000, st_gid\u003d1000, st_size\u003d389, st_atime\u003d1488232196, st_mtime\u003d1486791969, st_ctime\u003d1486791969)\n\u003e\u003e\u003e st2 \u003d os.statvfs(\"/home/vagrant/.ssh/authorized_keys\")\n\u003e\u003e\u003e st2\nposix.statvfs_result(f_bsize\u003d4096, f_frsize\u003d4096, f_blocks\u003d9817412, f_bfree\u003d9478084, f_bavail\u003d9478084, f_files\u003d19644416, f_ffree\u003d19609718, f_favail\u003d19609718, f_flag\u003d4096, f_namemax\u003d255)\n\nThe statvfs doesn\u0027t return the device id that file is on, which we are trying to compare. Let me know if I am wrong here?","commit_id":"23d6d9c04cf3fffda65e37961e1ed3201156ada8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"fbcf9a27bb7fb7b4915ca12b262ff35f07e37bfb","unresolved":false,"context_lines":[{"line_number":112,"context_line":""},{"line_number":113,"context_line":"A temporary directory that is relative to the ``filesystem_store_datadir``"},{"line_number":114,"context_line":"or ``filesystem_store_datadirs``. File system driver uses this as a temporary"},{"line_number":115,"context_line":"place to store the uploaded images moves it to final directory once the upload"},{"line_number":116,"context_line":"is complete."},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"Possible values:"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba2be162_30921ef7","line":115,"range":{"start_line":115,"start_character":34,"end_line":115,"end_character":35},"updated":"2017-03-02 23:14:18.000000000","message":"Nit: This last sentence does not sound right to me. Please add \u0027and\u0027 in the highlighted area if you are uploading another PS.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"85676da015c97cf00e0a9b2156c362c86ab95b49","unresolved":false,"context_lines":[{"line_number":328,"context_line":"                    self._check_write_permission(datadir)"},{"line_number":329,"context_line":"                    self._set_exec_permission(datadir)"},{"line_number":330,"context_line":"                else:"},{"line_number":331,"context_line":"                    msg \u003d _LI(\"Directory to write image files does not exist \""},{"line_number":332,"context_line":"                              \"(%s). Creating.\") % datadir"},{"line_number":333,"context_line":"                    LOG.info(msg)"},{"line_number":334,"context_line":"                    try:"},{"line_number":335,"context_line":"                        os.makedirs(datadir)"},{"line_number":336,"context_line":"                        self._check_write_permission(datadir)"},{"line_number":337,"context_line":"                        self._set_exec_permission(datadir)"},{"line_number":338,"context_line":"                    except (IOError, OSError):"},{"line_number":339,"context_line":"                        if os.path.exists(datadir):"},{"line_number":340,"context_line":"                            # NOTE(markwash): If the path now exists, some"},{"line_number":341,"context_line":"                            # other process must have beat us in the race"},{"line_number":342,"context_line":"                            # condition. But it doesn\u0027t hurt, so we can"},{"line_number":343,"context_line":"                            # safely ignore the error."},{"line_number":344,"context_line":"                            self._check_write_permission(datadir)"},{"line_number":345,"context_line":"                            self._set_exec_permission(datadir)"},{"line_number":346,"context_line":"                            continue"},{"line_number":347,"context_line":"                        reason \u003d _(\"Unable to create datadir: %s\") % datadir"},{"line_number":348,"context_line":"                        LOG.error(reason)"},{"line_number":349,"context_line":"                        raise exceptions.BadStoreConfiguration("},{"line_number":350,"context_line":"                            store_name\u003d\"filesystem\", reason\u003dreason)"},{"line_number":351,"context_line":""},{"line_number":352,"context_line":"    def _validate_metadata(self, metadata_file):"},{"line_number":353,"context_line":"        \"\"\"Validate metadata against json schema."}],"source_content_type":"text/x-python","patch_set":16,"id":"9a30ddce_1be78b13","line":350,"range":{"start_line":331,"start_character":0,"end_line":350,"end_character":67},"updated":"2017-03-06 23:52:53.000000000","message":"One consideration, this logic could easily be encapsulated into another function, just to shorten this function and improve readability.\n\nIt appears that you can then replace the continue with a return and maintain the current behavior unless I\u0027m missing something.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"96a852f4ef9bdff0c9d161d6a6908416b00b3fb8","unresolved":false,"context_lines":[{"line_number":328,"context_line":"                    self._check_write_permission(datadir)"},{"line_number":329,"context_line":"                    self._set_exec_permission(datadir)"},{"line_number":330,"context_line":"                else:"},{"line_number":331,"context_line":"                    msg \u003d _LI(\"Directory to write image files does not exist \""},{"line_number":332,"context_line":"                              \"(%s). Creating.\") % datadir"},{"line_number":333,"context_line":"                    LOG.info(msg)"},{"line_number":334,"context_line":"                    try:"},{"line_number":335,"context_line":"                        os.makedirs(datadir)"},{"line_number":336,"context_line":"                        self._check_write_permission(datadir)"},{"line_number":337,"context_line":"                        self._set_exec_permission(datadir)"},{"line_number":338,"context_line":"                    except (IOError, OSError):"},{"line_number":339,"context_line":"                        if os.path.exists(datadir):"},{"line_number":340,"context_line":"                            # NOTE(markwash): If the path now exists, some"},{"line_number":341,"context_line":"                            # other process must have beat us in the race"},{"line_number":342,"context_line":"                            # condition. But it doesn\u0027t hurt, so we can"},{"line_number":343,"context_line":"                            # safely ignore the error."},{"line_number":344,"context_line":"                            self._check_write_permission(datadir)"},{"line_number":345,"context_line":"                            self._set_exec_permission(datadir)"},{"line_number":346,"context_line":"                            continue"},{"line_number":347,"context_line":"                        reason \u003d _(\"Unable to create datadir: %s\") % datadir"},{"line_number":348,"context_line":"                        LOG.error(reason)"},{"line_number":349,"context_line":"                        raise exceptions.BadStoreConfiguration("},{"line_number":350,"context_line":"                            store_name\u003d\"filesystem\", reason\u003dreason)"},{"line_number":351,"context_line":""},{"line_number":352,"context_line":"    def _validate_metadata(self, metadata_file):"},{"line_number":353,"context_line":"        \"\"\"Validate metadata against json schema."}],"source_content_type":"text/x-python","patch_set":16,"id":"9a30ddce_aa9fd901","line":350,"range":{"start_line":331,"start_character":0,"end_line":350,"end_character":67},"in_reply_to":"9a30ddce_1be78b13","updated":"2017-03-07 20:43:57.000000000","message":"Since this function is named create_image_directories carving out this logic to another function doesn\u0027t make sense IMO.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"67f8a998d32e784a5dae50fad166c3c4c563c05d","unresolved":false,"context_lines":[{"line_number":328,"context_line":"                    self._check_write_permission(datadir)"},{"line_number":329,"context_line":"                    self._set_exec_permission(datadir)"},{"line_number":330,"context_line":"                else:"},{"line_number":331,"context_line":"                    msg \u003d _LI(\"Directory to write image files does not exist \""},{"line_number":332,"context_line":"                              \"(%s). Creating.\") % datadir"},{"line_number":333,"context_line":"                    LOG.info(msg)"},{"line_number":334,"context_line":"                    try:"},{"line_number":335,"context_line":"                        os.makedirs(datadir)"},{"line_number":336,"context_line":"                        self._check_write_permission(datadir)"},{"line_number":337,"context_line":"                        self._set_exec_permission(datadir)"},{"line_number":338,"context_line":"                    except (IOError, OSError):"},{"line_number":339,"context_line":"                        if os.path.exists(datadir):"},{"line_number":340,"context_line":"                            # NOTE(markwash): If the path now exists, some"},{"line_number":341,"context_line":"                            # other process must have beat us in the race"},{"line_number":342,"context_line":"                            # condition. But it doesn\u0027t hurt, so we can"},{"line_number":343,"context_line":"                            # safely ignore the error."},{"line_number":344,"context_line":"                            self._check_write_permission(datadir)"},{"line_number":345,"context_line":"                            self._set_exec_permission(datadir)"},{"line_number":346,"context_line":"                            continue"},{"line_number":347,"context_line":"                        reason \u003d _(\"Unable to create datadir: %s\") % datadir"},{"line_number":348,"context_line":"                        LOG.error(reason)"},{"line_number":349,"context_line":"                        raise exceptions.BadStoreConfiguration("},{"line_number":350,"context_line":"                            store_name\u003d\"filesystem\", reason\u003dreason)"},{"line_number":351,"context_line":""},{"line_number":352,"context_line":"    def _validate_metadata(self, metadata_file):"},{"line_number":353,"context_line":"        \"\"\"Validate metadata against json schema."}],"source_content_type":"text/x-python","patch_set":16,"id":"1a1ced50_eacf6853","line":350,"range":{"start_line":331,"start_character":0,"end_line":350,"end_character":67},"in_reply_to":"9a30ddce_aa9fd901","updated":"2017-03-22 21:50:24.000000000","message":"I\u0027ll buy that argument, but how about moving these two duplicated lines outside of the if/else. This allows you to invert the logic of the if clause, and simplify things a touch.\n\n  self._check_write_permission(datadir)\n  self._set_exec_permission(datadir)","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"85676da015c97cf00e0a9b2156c362c86ab95b49","unresolved":false,"context_lines":[{"line_number":699,"context_line":"        if os.path.exists(filepath):"},{"line_number":700,"context_line":"            raise exceptions.Duplicate(image\u003dfilepath)"},{"line_number":701,"context_line":""},{"line_number":702,"context_line":"        if os.stat(os.path.dirname(filepath)).st_dev !\u003d \\"},{"line_number":703,"context_line":"                os.stat(os.path.dirname(temppath)).st_dev:"},{"line_number":704,"context_line":"            # If filepath and temppath are in different filesystem"},{"line_number":705,"context_line":"            # os.rename fails and we don\u0027t achieve atomicity in"},{"line_number":706,"context_line":"            # uploads, log a warning and go with normal upload"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a30ddce_1b414b3a","line":703,"range":{"start_line":702,"start_character":11,"end_line":703,"end_character":57},"updated":"2017-03-06 23:52:53.000000000","message":"as a nit, OpenStack prefers parens ( ) to  backslash \\ for multiline ops\n\nas a general comment, this boolean is quite opaque. It would be improved if there was a more expressive way to evaluate this, such as the following:\n\n  def _device_of_inode(target):\n    return os.stat(os.path.dirname(target)).st_dev\n\n  # [...]\n\n  if (_device_of_inode(filepath) !\u003d\n      _device_of_inode(temppath)):\n\nI\u0027m not convinced this is needed, but it would likely improve the readability of the code, so offering as a consideration.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"85676da015c97cf00e0a9b2156c362c86ab95b49","unresolved":false,"context_lines":[{"line_number":714,"context_line":"        checksum \u003d hashlib.md5()"},{"line_number":715,"context_line":"        bytes_written \u003d 0"},{"line_number":716,"context_line":"        try:"},{"line_number":717,"context_line":"            with open(temppath, \u0027wb\u0027) as f:"},{"line_number":718,"context_line":"                for buf in utils.chunkreadable(image_file,"},{"line_number":719,"context_line":"                                               self.WRITE_CHUNKSIZE):"},{"line_number":720,"context_line":"                    bytes_written +\u003d len(buf)"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a30ddce_5b902396","line":717,"updated":"2017-03-06 23:52:53.000000000","message":"at, and below, this point the use of temppath as a meaningful variable is somewhat deceptive, because the temppath may actually be the final filepath. This wouldn\u0027t be a problem except for the fact that this method has grown quite long.\n\nAs a consideration, perhaps use writepath or returning to using filepath would be clearer (though the latter would invert some prior logic, for better or worse).","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"96a852f4ef9bdff0c9d161d6a6908416b00b3fb8","unresolved":false,"context_lines":[{"line_number":714,"context_line":"        checksum \u003d hashlib.md5()"},{"line_number":715,"context_line":"        bytes_written \u003d 0"},{"line_number":716,"context_line":"        try:"},{"line_number":717,"context_line":"            with open(temppath, \u0027wb\u0027) as f:"},{"line_number":718,"context_line":"                for buf in utils.chunkreadable(image_file,"},{"line_number":719,"context_line":"                                               self.WRITE_CHUNKSIZE):"},{"line_number":720,"context_line":"                    bytes_written +\u003d len(buf)"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a30ddce_5ea1bea8","line":717,"in_reply_to":"9a30ddce_5b902396","updated":"2017-03-07 20:43:57.000000000","message":"Good point I will see if I can improve readability of this code a bit.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"85676da015c97cf00e0a9b2156c362c86ab95b49","unresolved":false,"context_lines":[{"line_number":740,"context_line":"                   \u0027checksum_hex\u0027: checksum})"},{"line_number":741,"context_line":""},{"line_number":742,"context_line":"        if temppath !\u003d filepath:"},{"line_number":743,"context_line":"            try:"},{"line_number":744,"context_line":"                LOG.debug(_(\"Moving %(temppath)s into place as %(filepath)s\")"},{"line_number":745,"context_line":"                          % dict(temppath\u003dtemppath, filepath\u003dfilepath))"},{"line_number":746,"context_line":"                os.rename(temppath, filepath)"},{"line_number":747,"context_line":"            except Exception:"},{"line_number":748,"context_line":"                with excutils.save_and_reraise_exception():"},{"line_number":749,"context_line":"                    self._delete_partial(temppath, image_id)"},{"line_number":750,"context_line":""},{"line_number":751,"context_line":"        checksum_hex \u003d checksum.hexdigest()"},{"line_number":752,"context_line":"        metadata \u003d self._get_metadata(filepath)"}],"source_content_type":"text/x-python","patch_set":16,"id":"9a30ddce_bb3bdf9b","line":749,"range":{"start_line":743,"start_character":0,"end_line":749,"end_character":60},"updated":"2017-03-06 23:52:53.000000000","message":"One more consideration, this logic could easily be encapsulated into another function, just to shorten this function and improve readability.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"15931ebacac58e0c22ea848f5bceb83958297918","unresolved":false,"context_lines":[{"line_number":332,"context_line":"                        os.makedirs(datadir)"},{"line_number":333,"context_line":"                    except (IOError, OSError):"},{"line_number":334,"context_line":"                        if not os.path.exists(datadir):"},{"line_number":335,"context_line":"                            # NOTE(markwash): If the path now exists, some"},{"line_number":336,"context_line":"                            # other process must have beat us in the race"},{"line_number":337,"context_line":"                            # condition. But it doesn\u0027t hurt, so we can"},{"line_number":338,"context_line":"                            # safely ignore the error."},{"line_number":339,"context_line":"                            reason \u003d (_(\"Unable to create datadir: %s\") %"},{"line_number":340,"context_line":"                                      datadir)"},{"line_number":341,"context_line":"                            LOG.error(reason)"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_7e7d286d","line":338,"range":{"start_line":335,"start_character":28,"end_line":338,"end_character":54},"updated":"2017-03-30 23:44:34.000000000","message":"Since you moved the piece of code that was doing a \u0027continue\u0027, this comment becomes inappropriate here.\nI think we should implement the comment (as previous PS).","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"320fb9546fcba581812039260e84ab8fc7222692","unresolved":false,"context_lines":[{"line_number":332,"context_line":"                        os.makedirs(datadir)"},{"line_number":333,"context_line":"                    except (IOError, OSError):"},{"line_number":334,"context_line":"                        if not os.path.exists(datadir):"},{"line_number":335,"context_line":"                            # NOTE(markwash): If the path now exists, some"},{"line_number":336,"context_line":"                            # other process must have beat us in the race"},{"line_number":337,"context_line":"                            # condition. But it doesn\u0027t hurt, so we can"},{"line_number":338,"context_line":"                            # safely ignore the error."},{"line_number":339,"context_line":"                            reason \u003d (_(\"Unable to create datadir: %s\") %"},{"line_number":340,"context_line":"                                      datadir)"},{"line_number":341,"context_line":"                            LOG.error(reason)"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_fcb7e4d7","line":338,"range":{"start_line":335,"start_character":28,"end_line":338,"end_character":54},"in_reply_to":"dfeb2761_7e7d286d","updated":"2017-03-31 17:14:27.000000000","message":"Done","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"15931ebacac58e0c22ea848f5bceb83958297918","unresolved":false,"context_lines":[{"line_number":342,"context_line":"                            raise exceptions.BadStoreConfiguration("},{"line_number":343,"context_line":"                                store_name\u003d\"filesystem\", reason\u003dreason)"},{"line_number":344,"context_line":""},{"line_number":345,"context_line":"                self._check_write_permission(datadir)"},{"line_number":346,"context_line":"                self._set_exec_permission(datadir)"},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"    def _validate_metadata(self, metadata_file):"},{"line_number":349,"context_line":"        \"\"\"Validate metadata against json schema."}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_5ed02451","line":346,"range":{"start_line":345,"start_character":16,"end_line":346,"end_character":50},"updated":"2017-03-30 23:44:34.000000000","message":"Not sure if we should undo that part of logic (safely ignoring an error like what the comment says)\nIf that is the case, please move this back inside of try and except blocks.\nElse, if we decide for some reason to skip that, please get rid of the comment and move these 2 lines inside en else block. (try-except-else)","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"320fb9546fcba581812039260e84ab8fc7222692","unresolved":false,"context_lines":[{"line_number":342,"context_line":"                            raise exceptions.BadStoreConfiguration("},{"line_number":343,"context_line":"                                store_name\u003d\"filesystem\", reason\u003dreason)"},{"line_number":344,"context_line":""},{"line_number":345,"context_line":"                self._check_write_permission(datadir)"},{"line_number":346,"context_line":"                self._set_exec_permission(datadir)"},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"    def _validate_metadata(self, metadata_file):"},{"line_number":349,"context_line":"        \"\"\"Validate metadata against json schema."}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_693d54ba","line":346,"range":{"start_line":345,"start_character":16,"end_line":346,"end_character":50},"in_reply_to":"dfeb2761_5ed02451","updated":"2017-03-31 17:14:27.000000000","message":"These lines need to be executed either path exist or doesn\u0027t exist, so they are outside of if condition. I.e we check permissions either after we create the directory or if directory already exists. This change simplifies the logic and avoids repeating check write permissions. This is  suggested by Steve on a prev. PS","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"15931ebacac58e0c22ea848f5bceb83958297918","unresolved":false,"context_lines":[{"line_number":711,"context_line":"            raise exceptions.Duplicate(image\u003dfilepath)"},{"line_number":712,"context_line":""},{"line_number":713,"context_line":"        if self._files_not_on_same_device(filepath, temppath):"},{"line_number":714,"context_line":"            temppath \u003d filepath"},{"line_number":715,"context_line":""},{"line_number":716,"context_line":"        checksum \u003d hashlib.md5()"},{"line_number":717,"context_line":"        bytes_written \u003d 0"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_1ee3bc07","line":714,"range":{"start_line":714,"start_character":12,"end_line":714,"end_character":20},"updated":"2017-03-30 23:44:34.000000000","message":"I think Steve suggested that usage of the variable \u0027temppath\u0027 might be confusing here.\n\nHow about using a variable like \"write_path\"?","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"15931ebacac58e0c22ea848f5bceb83958297918","unresolved":false,"context_lines":[{"line_number":710,"context_line":"        if os.path.exists(filepath):"},{"line_number":711,"context_line":"            raise exceptions.Duplicate(image\u003dfilepath)"},{"line_number":712,"context_line":""},{"line_number":713,"context_line":"        if self._files_not_on_same_device(filepath, temppath):"},{"line_number":714,"context_line":"            temppath \u003d filepath"},{"line_number":715,"context_line":""},{"line_number":716,"context_line":"        checksum \u003d hashlib.md5()"},{"line_number":717,"context_line":"        bytes_written \u003d 0"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_3ecfa080","line":714,"range":{"start_line":713,"start_character":8,"end_line":714,"end_character":31},"updated":"2017-03-30 23:44:34.000000000","message":"if self._files_not_on_same_device(filepath, temppath):\n      write_path \u003d filepath\n  else:\n      write_path \u003d filepath","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":12637,"name":"Srinivas Sakhamuri","email":"srini.openstack@gmail.com","username":"srsakhamuri"},"change_message_id":"320fb9546fcba581812039260e84ab8fc7222692","unresolved":false,"context_lines":[{"line_number":710,"context_line":"        if os.path.exists(filepath):"},{"line_number":711,"context_line":"            raise exceptions.Duplicate(image\u003dfilepath)"},{"line_number":712,"context_line":""},{"line_number":713,"context_line":"        if self._files_not_on_same_device(filepath, temppath):"},{"line_number":714,"context_line":"            temppath \u003d filepath"},{"line_number":715,"context_line":""},{"line_number":716,"context_line":"        checksum \u003d hashlib.md5()"},{"line_number":717,"context_line":"        bytes_written \u003d 0"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_3cac2c8b","line":714,"range":{"start_line":713,"start_character":8,"end_line":714,"end_character":31},"in_reply_to":"dfeb2761_3ecfa080","updated":"2017-03-31 17:14:27.000000000","message":"I gave it a thought about using write_path for storing the temppath or filepath, But introducing another variable here may add to further confusion IMO. I will add a comment that says why we are assigning a filepath to temppath.","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"15931ebacac58e0c22ea848f5bceb83958297918","unresolved":false,"context_lines":[{"line_number":716,"context_line":"        checksum \u003d hashlib.md5()"},{"line_number":717,"context_line":"        bytes_written \u003d 0"},{"line_number":718,"context_line":"        try:"},{"line_number":719,"context_line":"            with open(temppath, \u0027wb\u0027) as f:"},{"line_number":720,"context_line":"                for buf in utils.chunkreadable(image_file,"},{"line_number":721,"context_line":"                                               self.WRITE_CHUNKSIZE):"},{"line_number":722,"context_line":"                    bytes_written +\u003d len(buf)"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_de1794ed","line":719,"range":{"start_line":719,"start_character":22,"end_line":719,"end_character":30},"updated":"2017-03-30 23:44:34.000000000","message":"s/temppath/write_path","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"15931ebacac58e0c22ea848f5bceb83958297918","unresolved":false,"context_lines":[{"line_number":726,"context_line":"                    f.write(buf)"},{"line_number":727,"context_line":"        except IOError as e:"},{"line_number":728,"context_line":"            if e.errno !\u003d errno.EACCES:"},{"line_number":729,"context_line":"                self._delete_partial(temppath, image_id)"},{"line_number":730,"context_line":"            errors \u003d {errno.EFBIG: exceptions.StorageFull(),"},{"line_number":731,"context_line":"                      errno.ENOSPC: exceptions.StorageFull(),"},{"line_number":732,"context_line":"                      errno.EACCES: exceptions.StorageWriteDenied()}"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_9ea58cb8","line":729,"range":{"start_line":729,"start_character":37,"end_line":729,"end_character":45},"updated":"2017-03-30 23:44:34.000000000","message":"s/temppath/write_path","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"15931ebacac58e0c22ea848f5bceb83958297918","unresolved":false,"context_lines":[{"line_number":733,"context_line":"            raise errors.get(e.errno, e)"},{"line_number":734,"context_line":"        except Exception:"},{"line_number":735,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":736,"context_line":"                self._delete_partial(temppath, image_id)"},{"line_number":737,"context_line":""},{"line_number":738,"context_line":"        LOG.debug(_(\"Wrote %(bytes_written)d bytes to %(temppath)s with \""},{"line_number":739,"context_line":"                    \"checksum %(checksum_hex)s\"),"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_5eaf0494","line":736,"range":{"start_line":736,"start_character":37,"end_line":736,"end_character":45},"updated":"2017-03-30 23:44:34.000000000","message":"s/temppath/write_path","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"15931ebacac58e0c22ea848f5bceb83958297918","unresolved":false,"context_lines":[{"line_number":741,"context_line":"                   \u0027temppath\u0027: temppath,"},{"line_number":742,"context_line":"                   \u0027checksum_hex\u0027: checksum})"},{"line_number":743,"context_line":""},{"line_number":744,"context_line":"        if temppath !\u003d filepath:"},{"line_number":745,"context_line":"            try:"},{"line_number":746,"context_line":"                LOG.debug(_(\"Moving %(temppath)s into place as %(filepath)s\")"},{"line_number":747,"context_line":"                          % dict(temppath\u003dtemppath, filepath\u003dfilepath))"}],"source_content_type":"text/x-python","patch_set":18,"id":"dfeb2761_1e917c53","line":744,"range":{"start_line":744,"start_character":11,"end_line":744,"end_character":19},"updated":"2017-03-30 23:44:34.000000000","message":"write_path !\u003d filepath","commit_id":"5142cd4397463f6c165d9c53fd49ded19ebaf3c8"}],"glance_store/tests/unit/test_filesystem_store.py":[{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"7f9da166d70f13e1fc202b81a93832fd35187c1a","unresolved":false,"context_lines":[{"line_number":50,"context_line":"        self.config(filesystem_store_datadir\u003dself.test_dir,"},{"line_number":51,"context_line":"                    stores\u003d[\u0027glance.store.filesystem.Store\u0027],"},{"line_number":52,"context_line":"                    group\u003d\"glance_store\")"},{"line_number":53,"context_line":"        self.config(filesystem_store_tempdir\u003d\u0027.upload\u0027)"},{"line_number":54,"context_line":"        self.store.configure()"},{"line_number":55,"context_line":"        self.register_store_schemes(self.store, \u0027file\u0027)"},{"line_number":56,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"da6895a0_18848ed2","line":53,"updated":"2016-10-24 18:52:30.000000000","message":"do not make this change here. see below.","commit_id":"961a6a28a6a499420a591fdbccf6645204c55a8a"},{"author":{"_account_id":12807,"name":"Steve Lewis (stevelle)","email":"stevelle@gmail.com","username":"stevelle"},"change_message_id":"7f9da166d70f13e1fc202b81a93832fd35187c1a","unresolved":false,"context_lines":[{"line_number":198,"context_line":"        verifier.update.assert_called_with(file_contents)"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"    def test_add_atomic(self):"},{"line_number":201,"context_line":"        self.config(filesystem_store_tempdir\u003d\u0027.upload\u0027)"},{"line_number":202,"context_line":"        self.test_add()"},{"line_number":203,"context_line":""},{"line_number":204,"context_line":"    def test_add_check_metadata_with_invalid_mountpoint_location(self):"}],"source_content_type":"text/x-python","patch_set":13,"id":"da6895a0_98e6deb2","line":201,"updated":"2016-10-24 18:52:30.000000000","message":"After this line, before running the test_add case, I think you should add \n\n    self.store.configure() \n\nto apply the configuration change to storage (like line 54) in order to test this functionality.\n\nWhat you are doing on line 53 is causing all tests to use the tempdir. While that may be effective for testing the new functionality it provides no test coverage for the case where this configuration is left with a default value.","commit_id":"961a6a28a6a499420a591fdbccf6645204c55a8a"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"de78e646c0c296b761f08985b781ffbea6135533","unresolved":false,"context_lines":[{"line_number":199,"context_line":"        self.config(filesystem_store_tempdir\u003d\u0027.upload\u0027)"},{"line_number":200,"context_line":"        self.store.configure()"},{"line_number":201,"context_line":"        self.test_add()"},{"line_number":202,"context_line":""},{"line_number":203,"context_line":"    def test_add_check_metadata_with_invalid_mountpoint_location(self):"},{"line_number":204,"context_line":"        in_metadata \u003d [{\u0027id\u0027: \u0027abcdefg\u0027,"},{"line_number":205,"context_line":"                       \u0027mountpoint\u0027: \u0027/xyz/images\u0027}]"}],"source_content_type":"text/x-python","patch_set":15,"id":"fa31d9ce_e7593590","line":202,"updated":"2017-02-10 02:46:02.000000000","message":"Can we use some tests for making sure that:\n#1  Our add() proceeds with direct filepath instead of temppath after our check in L702\n#2 and that os.rename went through\n#3 If not, _delete_partial was called.","commit_id":"23d6d9c04cf3fffda65e37961e1ed3201156ada8"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"fbcf9a27bb7fb7b4915ca12b262ff35f07e37bfb","unresolved":false,"context_lines":[{"line_number":207,"context_line":"            mock_rename.assert_called_once()"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":"    def test_add_atomic_delete_partial(self):"},{"line_number":210,"context_line":"        self.config(filesystem_store_tempdir\u003d\u0027.upload\u0027)"},{"line_number":211,"context_line":"        self.store.configure()"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"        class MyRenameException(Exception):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba2be162_53307ce8","line":210,"updated":"2017-03-02 23:14:18.000000000","message":"Please consider adding a comment line that says something like this test case is to ensure that in case of a failure while moving the image form the temp dir to final dir, the partial image if any is deleted.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"fbcf9a27bb7fb7b4915ca12b262ff35f07e37bfb","unresolved":false,"context_lines":[{"line_number":222,"context_line":"            mock_delete_partial.assert_called_once()"},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"    def test_atomic_temp_failed(self):"},{"line_number":225,"context_line":"        self.config(filesystem_store_tempdir\u003d\u0027.upload\u0027)"},{"line_number":226,"context_line":"        self.store.configure()"},{"line_number":227,"context_line":""},{"line_number":228,"context_line":"        os_stat_orig \u003d os.stat"}],"source_content_type":"text/x-python","patch_set":16,"id":"ba2be162_13afe4e2","line":225,"updated":"2017-03-02 23:14:18.000000000","message":"Same. A comment that says that in case of different filesystems, add proceeds normally.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"},{"author":{"_account_id":21722,"name":"Dharini Chandrasekar","email":"dharini.chandrasekar@intel.com","username":"dharinic"},"change_message_id":"fbcf9a27bb7fb7b4915ca12b262ff35f07e37bfb","unresolved":false,"context_lines":[{"line_number":248,"context_line":"        with mock.patch.object(os, \"stat\") as mock_stat,\\"},{"line_number":249,"context_line":"                mock.patch.object(os, \"rename\") as mock_rename:"},{"line_number":250,"context_line":"            mock_stat.side_effect \u003d fake_stat"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"            self.test_add()"},{"line_number":253,"context_line":"            mock_rename.assert_not_called()"},{"line_number":254,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"ba2be162_33e6a844","line":251,"updated":"2017-03-02 23:14:18.000000000","message":"You could consider asserting: LOG.assert_called_once_with(\u003c\u003e). But not stressing.","commit_id":"1645e1ace6de97812aa679e47037c96c622d31fa"}]}