)]}'
{"glance/api/v1/images.py":[{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"742a7125ab51b6d5e1d006fd8b7d04a08145ec3b","unresolved":false,"context_lines":[{"line_number":455,"context_line":"        source \u003d image_meta.get(\u0027location\u0027)"},{"line_number":456,"context_line":"        if source is not None:"},{"line_number":457,"context_line":"            self._enforce(req, \u0027set_image_location\u0027)"},{"line_number":458,"context_line":"        else:"},{"line_number":459,"context_line":"            if \u0027location\u0027 in image_meta:"},{"line_number":460,"context_line":"                # ensure that the location is not empty string"},{"line_number":461,"context_line":"                msg \u003d _(\"Location must be non-empty string.\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_0f5db646","line":458,"updated":"2015-09-23 08:56:12.000000000","message":"It would be better to move this check for an empty string into either _validate_source or validate_external_location. It is a \u0027validation\u0027 after all.\n\nMoving the check there should catch both this case and the case where \"x-glance-api-copy-from\" is an empty string in one central place.","commit_id":"f41afdbd68ed7077fef1d3af84cc02ac1a9ae1d3"},{"author":{"_account_id":15054,"name":"wangxiyuan","email":"wangxiyuan1007@gmail.com","username":"wangxiyuan"},"change_message_id":"8d692e4c21ab2da48fe34518381d9a623a97b392","unresolved":false,"context_lines":[{"line_number":455,"context_line":"        source \u003d image_meta.get(\u0027location\u0027)"},{"line_number":456,"context_line":"        if source is not None:"},{"line_number":457,"context_line":"            self._enforce(req, \u0027set_image_location\u0027)"},{"line_number":458,"context_line":"        else:"},{"line_number":459,"context_line":"            if \u0027location\u0027 in image_meta:"},{"line_number":460,"context_line":"                # ensure that the location is not empty string"},{"line_number":461,"context_line":"                msg \u003d _(\"Location must be non-empty string.\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_529b5166","line":458,"in_reply_to":"ba15a1d1_0f5db646","updated":"2015-09-23 09:11:29.000000000","message":"+1, and use elif looks more clean.","commit_id":"f41afdbd68ed7077fef1d3af84cc02ac1a9ae1d3"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"e047a7a8a07e9faceb5531dc48e754a057dfc917","unresolved":false,"context_lines":[{"line_number":455,"context_line":"        source \u003d image_meta.get(\u0027location\u0027)"},{"line_number":456,"context_line":"        if source is not None:"},{"line_number":457,"context_line":"            self._enforce(req, \u0027set_image_location\u0027)"},{"line_number":458,"context_line":"        else:"},{"line_number":459,"context_line":"            if \u0027location\u0027 in image_meta:"},{"line_number":460,"context_line":"                # ensure that the location is not empty string"},{"line_number":461,"context_line":"                msg \u003d _(\"Location must be non-empty string.\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba15a1d1_e5b18190","line":458,"in_reply_to":"ba15a1d1_529b5166","updated":"2015-09-23 09:41:05.000000000","message":"Unfortunately, it is not so simple as could be because Glance should accept empty values for location and copy-from when specifying image. It seems that I need to pass image_meta to validation method in order to check if the empty value is present in it. Anyway, thanks for pointing it out. Looks like I need more refactoring on this.","commit_id":"f41afdbd68ed7077fef1d3af84cc02ac1a9ae1d3"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"42f8741a9f2243c733d2bc7d8be9f011260cefa0","unresolved":false,"context_lines":[{"line_number":670,"context_line":"            \u0027location\u0027: location_data[\u0027url\u0027],"},{"line_number":671,"context_line":"            \u0027status\u0027: \u0027active\u0027,"},{"line_number":672,"context_line":"            \u0027location_data\u0027: [location_data]"},{"line_number":673,"context_line":"        }"},{"line_number":674,"context_line":""},{"line_number":675,"context_line":"        try:"},{"line_number":676,"context_line":"            s \u003d from_state"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba15a1d1_7d70a1a4","line":673,"updated":"2015-09-23 11:05:42.000000000","message":"This change is already in the master, please rebase with the latest master.\n\nRef, https://review.openstack.org/#/c/210303/2/glance/api/v1/images.py","commit_id":"7b0f11eab931319e57c438b654611100fe461f2d"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"e8254a6b065b90094d2d6b38aa3f79aa5c720b57","unresolved":false,"context_lines":[{"line_number":670,"context_line":"            \u0027location\u0027: location_data[\u0027url\u0027],"},{"line_number":671,"context_line":"            \u0027status\u0027: \u0027active\u0027,"},{"line_number":672,"context_line":"            \u0027location_data\u0027: [location_data]"},{"line_number":673,"context_line":"        }"},{"line_number":674,"context_line":""},{"line_number":675,"context_line":"        try:"},{"line_number":676,"context_line":"            s \u003d from_state"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba15a1d1_8ed45d20","line":673,"in_reply_to":"ba15a1d1_7d70a1a4","updated":"2015-09-23 11:52:28.000000000","message":"Done","commit_id":"7b0f11eab931319e57c438b654611100fe461f2d"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"b8cf614b072013f587ff86ea8fb1bb8e96566dc9","unresolved":false,"context_lines":[{"line_number":438,"context_line":"        or copy-from headers) are supported. Otherwise we reject"},{"line_number":439,"context_line":"        with 400 \"Bad Request\"."},{"line_number":440,"context_line":"        \"\"\""},{"line_number":441,"context_line":"        if source and store_utils.validate_external_location(source):"},{"line_number":442,"context_line":"            return source"},{"line_number":443,"context_line":"        else:"},{"line_number":444,"context_line":"            msg \u003d _(\"External sources are not supported: \u0027%s\u0027\") % source"}],"source_content_type":"text/x-python","patch_set":3,"id":"ba15a1d1_c99c8f6c","line":441,"updated":"2015-09-23 12:07:49.000000000","message":"What if source is \"\" or None then it will go in else part and msg will be formed like External sources are not supported: \u0027\u0027 or External sources are not supported: None","commit_id":"de407acaaaaebdee8f52d91db5c1716c1e98e85b"},{"author":{"_account_id":15054,"name":"wangxiyuan","email":"wangxiyuan1007@gmail.com","username":"wangxiyuan"},"change_message_id":"6757a2c51790754695e439c51a707edd06b9aaab","unresolved":false,"context_lines":[{"line_number":442,"context_line":"            return source"},{"line_number":443,"context_line":"        else:"},{"line_number":444,"context_line":"            msg \u003d (_(\"External sources are not supported: \u0027%s\u0027\") % source"},{"line_number":445,"context_line":"                   if source else _(\"External source should not be empty\"))"},{"line_number":446,"context_line":"            LOG.warn(msg)"},{"line_number":447,"context_line":"            raise HTTPBadRequest(explanation\u003dmsg,"},{"line_number":448,"context_line":"                                 request\u003dreq,"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba15a1d1_337f91e1","line":445,"updated":"2015-09-24 06:18:22.000000000","message":"Information could be more clear,like:\n\"External source %s should not be empty\" % source.","commit_id":"908cc40b6f1215e16cca1f33bc668cde73b823e5"},{"author":{"_account_id":15054,"name":"wangxiyuan","email":"wangxiyuan1007@gmail.com","username":"wangxiyuan"},"change_message_id":"0b297c456645cc20305b940f271457eba7848fff","unresolved":false,"context_lines":[{"line_number":442,"context_line":"            return source"},{"line_number":443,"context_line":"        else:"},{"line_number":444,"context_line":"            msg \u003d (_(\"External sources are not supported: \u0027%s\u0027\") % source"},{"line_number":445,"context_line":"                   if source else _(\"External source should not be empty\"))"},{"line_number":446,"context_line":"            LOG.warn(msg)"},{"line_number":447,"context_line":"            raise HTTPBadRequest(explanation\u003dmsg,"},{"line_number":448,"context_line":"                                 request\u003dreq,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a1a9d01_1adb391d","line":445,"in_reply_to":"9a1a9d01_57a3cc9e","updated":"2015-09-24 14:05:56.000000000","message":"My fault, I mean to show which is wrong, location or copy-from.","commit_id":"908cc40b6f1215e16cca1f33bc668cde73b823e5"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"6c42974ede883f397745f75159650094629ad49a","unresolved":false,"context_lines":[{"line_number":442,"context_line":"            return source"},{"line_number":443,"context_line":"        else:"},{"line_number":444,"context_line":"            msg \u003d (_(\"External sources are not supported: \u0027%s\u0027\") % source"},{"line_number":445,"context_line":"                   if source else _(\"External source should not be empty\"))"},{"line_number":446,"context_line":"            LOG.warn(msg)"},{"line_number":447,"context_line":"            raise HTTPBadRequest(explanation\u003dmsg,"},{"line_number":448,"context_line":"                                 request\u003dreq,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a1a9d01_57a3cc9e","line":445,"in_reply_to":"ba15a1d1_337f91e1","updated":"2015-09-24 13:47:49.000000000","message":"Hmm, doesn\u0027t seem so. Source could be None or \u0027\u0027, so there is no need to substitute external source into exception string.","commit_id":"908cc40b6f1215e16cca1f33bc668cde73b823e5"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"f6cd17404ed3f43133fdc624077b12386ebb30ee","unresolved":false,"context_lines":[{"line_number":442,"context_line":"            return source"},{"line_number":443,"context_line":"        else:"},{"line_number":444,"context_line":"            msg \u003d (_(\"External sources are not supported: \u0027%s\u0027\") % source"},{"line_number":445,"context_line":"                   if source else _(\"External source should not be empty\"))"},{"line_number":446,"context_line":"            LOG.warn(msg)"},{"line_number":447,"context_line":"            raise HTTPBadRequest(explanation\u003dmsg,"},{"line_number":448,"context_line":"                                 request\u003dreq,"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a8ffd7b_7baf98e3","line":445,"updated":"2015-12-01 19:25:00.000000000","message":"nit: Could you move this out of this parentheses? We\u0027ve been trying hard to keep the code readable and this nested if would not help with that!","commit_id":"955f2264590ff1d8f070852d28de4143ed37c80e"},{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"c54b645e498620419c89ce3d5c4cfdf5095d206d","unresolved":false,"context_lines":[{"line_number":438,"context_line":"        or copy-from headers) are supported. Otherwise we reject"},{"line_number":439,"context_line":"        with 400 \"Bad Request\"."},{"line_number":440,"context_line":"        \"\"\""},{"line_number":441,"context_line":"        if source and store_utils.validate_external_location(source):"},{"line_number":442,"context_line":"            return source"},{"line_number":443,"context_line":"        else:"},{"line_number":444,"context_line":"            if source:"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a740942_e9851907","line":441,"updated":"2015-12-04 16:09:49.000000000","message":"validate_external_location\u0027s job is to return True or False depending on whether \u0027source\u0027 is sane or not. It really should be able to do that if source is an empty string, ie:\n\n\n --- a/glance/common/store_utils.py\n +++ b/glance/common/store_utils.py\n @@ -132,6 +132,8 @@ def validate_external_location(uri):\n \n     # TODO(zhiyan): This function could be moved to glance_store.\n     # TODO(gm): Use a whitelist of allowed schemes\n +    if not uri:\n +        return False\n\nI\u0027d suggest adding the above, and we can then just make this:\n\nif store_utils.validate_external_location(source):","commit_id":"3722d68932ee80682a0068eeb2c127a406694099"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"95469b7b48dfa17643d7bc53880778c802c16ae8","unresolved":false,"context_lines":[{"line_number":438,"context_line":"        or copy-from headers) are supported. Otherwise we reject"},{"line_number":439,"context_line":"        with 400 \"Bad Request\"."},{"line_number":440,"context_line":"        \"\"\""},{"line_number":441,"context_line":"        if source and store_utils.validate_external_location(source):"},{"line_number":442,"context_line":"            return source"},{"line_number":443,"context_line":"        else:"},{"line_number":444,"context_line":"            if source:"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a740942_a4edb9f0","line":441,"in_reply_to":"7a740942_e9851907","updated":"2015-12-07 11:43:39.000000000","message":"Nice catch! Thanks.","commit_id":"3722d68932ee80682a0068eeb2c127a406694099"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"6143a7870c456656560df361f5d4a2f1e80f50a4","unresolved":false,"context_lines":[{"line_number":467,"context_line":"        \"\"\""},{"line_number":468,"context_line":"        if store_utils.validate_external_location(source):"},{"line_number":469,"context_line":"            return source"},{"line_number":470,"context_line":"        else:"},{"line_number":471,"context_line":"            if source:"},{"line_number":472,"context_line":"                msg \u003d _(\"External sources are not supported: \u0027%s\u0027\") % source"},{"line_number":473,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa69d971_4a499acf","line":470,"updated":"2016-01-08 11:35:13.000000000","message":"NIT: This else-block is redundant. We could use elif .. else or just if..else and drop the whole block as the functionality of this else is already filled the if above calling return","commit_id":"c2cbdd84e721271244dbf1c946e79fc2a0398420"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"37cfca9995f4ffec5593d5a71be6ad158ba63460","unresolved":false,"context_lines":[{"line_number":467,"context_line":"        \"\"\""},{"line_number":468,"context_line":"        if store_utils.validate_external_location(source):"},{"line_number":469,"context_line":"            return source"},{"line_number":470,"context_line":"        else:"},{"line_number":471,"context_line":"            if source:"},{"line_number":472,"context_line":"                msg \u003d _(\"External sources are not supported: \u0027%s\u0027\") % source"},{"line_number":473,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a68dd71_b7674f24","line":470,"in_reply_to":"9a68dd71_59e17499","updated":"2016-01-22 00:47:24.000000000","message":"FWIW, I noticed this when I reviewed this patch but I didn\u0027t think (and I still don\u0027t) it was worth holding off this patch on this.\n\nThese are the kind of cases that we as reviewers can either quickly fix on the patch, propose a follow-up patch for or simply live with it. Just my humble opinion.","commit_id":"c2cbdd84e721271244dbf1c946e79fc2a0398420"},{"author":{"_account_id":14676,"name":"Kairat Kushaev","email":"kkushaev@mirantis.com","username":"kairat_kushaev"},"change_message_id":"3dd23eb2fb5e536554024e29c72e89a8c6202ea6","unresolved":false,"context_lines":[{"line_number":467,"context_line":"        \"\"\""},{"line_number":468,"context_line":"        if store_utils.validate_external_location(source):"},{"line_number":469,"context_line":"            return source"},{"line_number":470,"context_line":"        else:"},{"line_number":471,"context_line":"            if source:"},{"line_number":472,"context_line":"                msg \u003d _(\"External sources are not supported: \u0027%s\u0027\") % source"},{"line_number":473,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa69d971_de88ac73","line":470,"in_reply_to":"fa69d971_4a499acf","updated":"2016-01-11 09:44:11.000000000","message":"Thanks for the comment. I think it is matter of style how to treat such cases. IMO, else statement is valid here cause it exposes the following logical structure:\nif valid:\n   # all good, return\nelse:\n   # we are in trouble, do something here.","commit_id":"c2cbdd84e721271244dbf1c946e79fc2a0398420"},{"author":{"_account_id":12000,"name":"Ian Cordasco","email":"sigmavirus24@gmail.com","username":"sigmavirus24"},"change_message_id":"d660eff0e17d05e25931fce3a811e27496487c62","unresolved":false,"context_lines":[{"line_number":467,"context_line":"        \"\"\""},{"line_number":468,"context_line":"        if store_utils.validate_external_location(source):"},{"line_number":469,"context_line":"            return source"},{"line_number":470,"context_line":"        else:"},{"line_number":471,"context_line":"            if source:"},{"line_number":472,"context_line":"                msg \u003d _(\"External sources are not supported: \u0027%s\u0027\") % source"},{"line_number":473,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a68dd71_59e17499","line":470,"in_reply_to":"fa69d971_de88ac73","updated":"2016-01-21 19:45:23.000000000","message":"Since you\u0027re returning from the if block, you do not even need an else. You can write this\n\n    if store_utils.validate_external_location(source):\n        return source\n\n    if source:\n        msg \u003d _(\"....\") % source\n    else:\n        msg \u003d _(\"....\")\n    LOG.warning(msg)\n    raise HTTPBadRequest(...)\n\nI haven\u0027t looked at store_utils.validate_external_location but will that handle false-y values of source? Is there a reason that was guarded against previously?","commit_id":"c2cbdd84e721271244dbf1c946e79fc2a0398420"}]}