)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"552574cff14124075bb52b081819bccf4cbbb04b","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Fix LIO target helper when missing targetcli"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"After patch to fix LIO target persistence was merged a bug was"},{"line_number":10,"context_line":"introduced where target creation/removal would result in an exception if"},{"line_number":11,"context_line":"targetcli is not installed on the system."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"7a016987_b6ca5f19","line":9,"updated":"2015-05-22 14:58:17.000000000","message":"Add a \u0027,\u0027 after \u0027was merged\u0027? The first sentence is a bit confusing.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a6ea0da01713152ca308b1c8625213d75689b850","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Fix LIO target helper when missing targetcli"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"After patch to fix LIO target persistence was merged a bug was"},{"line_number":10,"context_line":"introduced where target creation/removal would result in an exception if"},{"line_number":11,"context_line":"targetcli is not installed on the system."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"7a016987_46c35538","line":9,"in_reply_to":"7a016987_b6ca5f19","updated":"2015-05-25 12:28:44.000000000","message":"Done","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"552574cff14124075bb52b081819bccf4cbbb04b","unresolved":false,"context_lines":[{"line_number":10,"context_line":"introduced where target creation/removal would result in an exception if"},{"line_number":11,"context_line":"targetcli is not installed on the system."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This patch fixes this by checking if patch to the default file exists on"},{"line_number":14,"context_line":"the system and creating it if doesn\u0027t."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I5ecc1ae47482ea43a4890ccd81a4e872ab570069"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"7a016987_1676d3d6","line":13,"updated":"2015-05-22 14:58:17.000000000","message":"\u0027if patch to\u0027 \u003d\u003e \u0027if path to\u0027","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a6ea0da01713152ca308b1c8625213d75689b850","unresolved":false,"context_lines":[{"line_number":10,"context_line":"introduced where target creation/removal would result in an exception if"},{"line_number":11,"context_line":"targetcli is not installed on the system."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This patch fixes this by checking if patch to the default file exists on"},{"line_number":14,"context_line":"the system and creating it if doesn\u0027t."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I5ecc1ae47482ea43a4890ccd81a4e872ab570069"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"7a016987_26be19ae","line":13,"in_reply_to":"7a016987_1676d3d6","updated":"2015-05-25 12:28:44.000000000","message":"Done","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"}],"cinder/cmd/rtstool.py":[{"author":{"_account_id":1207,"name":"Duncan Thomas","email":"duncan.thomas@gmail.com","username":"duncan-thomas"},"change_message_id":"86d949e693a1c74564e7a10a236267aa286a4538","unresolved":false,"context_lines":[{"line_number":186,"context_line":""},{"line_number":187,"context_line":"            # With default file we ensure path exists and create it if doesn\u0027t"},{"line_number":188,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":189,"context_line":"                os.makedirs(path_to_file)"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        rtsroot.save_to_file(destination_file)"},{"line_number":192,"context_line":"    except (OSError, IOError) as exc:"}],"source_content_type":"text/x-python","patch_set":1,"id":"9af37de9_e9c0d9e2","line":189,"updated":"2015-05-10 16:41:02.000000000","message":"Given we\u0027re not running as root, why do we have permission to create this?","commit_id":"a380d85813c1b78aba5ecad6633c085e1335963c"},{"author":{"_account_id":1207,"name":"Duncan Thomas","email":"duncan.thomas@gmail.com","username":"duncan-thomas"},"change_message_id":"45e0a22101e4d426fab58418e1ae286388685d03","unresolved":false,"context_lines":[{"line_number":186,"context_line":""},{"line_number":187,"context_line":"            # With default file we ensure path exists and create it if doesn\u0027t"},{"line_number":188,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":189,"context_line":"                os.makedirs(path_to_file)"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        rtsroot.save_to_file(destination_file)"},{"line_number":192,"context_line":"    except (OSError, IOError) as exc:"}],"source_content_type":"text/x-python","patch_set":1,"id":"9af37de9_c4dbb2ec","line":189,"in_reply_to":"9af37de9_045e64e9","updated":"2015-05-11 09:07:56.000000000","message":"Yes, I think a comment will help people (like me) who don\u0027t have a total model for how this goes together in their head - it will certainly do no harm. \n\nOther than that, change looks good to me.","commit_id":"a380d85813c1b78aba5ecad6633c085e1335963c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c9cecad485b2552f78606252751a0cce1250874c","unresolved":false,"context_lines":[{"line_number":186,"context_line":""},{"line_number":187,"context_line":"            # With default file we ensure path exists and create it if doesn\u0027t"},{"line_number":188,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":189,"context_line":"                os.makedirs(path_to_file)"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        rtsroot.save_to_file(destination_file)"},{"line_number":192,"context_line":"    except (OSError, IOError) as exc:"}],"source_content_type":"text/x-python","patch_set":1,"id":"9af37de9_045e64e9","line":189,"in_reply_to":"9af37de9_e9c0d9e2","updated":"2015-05-10 17:31:35.000000000","message":"From Cinder\u0027s LIO target helper this is run as root: https://github.com/openstack/cinder/blob/master/cinder/volume/targets/lio.py#L82\n\nAnd if you run this manually from the command line without being root you will get an error, which is correct. Same behavior as when you try to create or destroy a target without being root.\n\nDo you think it\u0027s worth mentioning that in the comments to avoid confusion?","commit_id":"a380d85813c1b78aba5ecad6633c085e1335963c"},{"author":{"_account_id":1207,"name":"Duncan Thomas","email":"duncan.thomas@gmail.com","username":"duncan-thomas"},"change_message_id":"dd4eae5c600e97cf3ed2700e69559cd18171528d","unresolved":false,"context_lines":[{"line_number":1,"context_line":"#!/usr/bin/env python"},{"line_number":2,"context_line":""},{"line_number":3,"context_line":"# Copyright 2012 - 2013 Red Hat, Inc."},{"line_number":4,"context_line":"# All Rights Reserved."}],"source_content_type":"text/x-python","patch_set":2,"id":"9af37de9_20fee16b","side":"PARENT","line":1,"updated":"2015-05-11 13:35:54.000000000","message":"Was this remmoved intentionally? If so, please note why in the commit message","commit_id":"5afa79cd4b2724c1bb87258683af03f558959b47"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"79b83267260fe47762e7c9898f0947bb939bdfe5","unresolved":false,"context_lines":[{"line_number":1,"context_line":"#!/usr/bin/env python"},{"line_number":2,"context_line":""},{"line_number":3,"context_line":"# Copyright 2012 - 2013 Red Hat, Inc."},{"line_number":4,"context_line":"# All Rights Reserved."}],"source_content_type":"text/x-python","patch_set":2,"id":"9af37de9_b937a8e2","side":"PARENT","line":1,"in_reply_to":"9af37de9_20fee16b","updated":"2015-05-11 14:27:07.000000000","message":"Sh*t!! I don\u0027t know how that happened!!\n\nThank you for catching it.","commit_id":"5afa79cd4b2724c1bb87258683af03f558959b47"},{"author":{"_account_id":7219,"name":"git-harry","email":"git-harry@live.co.uk","username":"git-harry"},"change_message_id":"ff3765470693c8cb1234c9aab57bd8a803eacacc","unresolved":false,"context_lines":[{"line_number":180,"context_line":"def save_to_file(destination_file):"},{"line_number":181,"context_line":"    rtsroot \u003d rtslib.root.RTSRoot()"},{"line_number":182,"context_line":"    try:"},{"line_number":183,"context_line":"        # If default destination use rtslib default save file"},{"line_number":184,"context_line":"        if not destination_file:"},{"line_number":185,"context_line":"            destination_file \u003d rtslib.root.default_save_file"},{"line_number":186,"context_line":"            path_to_file \u003d os.path.dirname(destination_file)"}],"source_content_type":"text/x-python","patch_set":3,"id":"7ad8896b_0e1a0b6f","line":183,"updated":"2015-05-11 16:43:26.000000000","message":"I think it would make more sense to have a separate try/except block above this one for os.makedirs. It would clearly separate the two steps and a more specifc error message could be given to RtstoolError.","commit_id":"cf2377bc71fd5fc9f5ce74386800098e35c85482"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"210130ebb46dbce7a9acdadc83e03d7a0b4856e1","unresolved":false,"context_lines":[{"line_number":180,"context_line":"def save_to_file(destination_file):"},{"line_number":181,"context_line":"    rtsroot \u003d rtslib.root.RTSRoot()"},{"line_number":182,"context_line":"    try:"},{"line_number":183,"context_line":"        # If default destination use rtslib default save file"},{"line_number":184,"context_line":"        if not destination_file:"},{"line_number":185,"context_line":"            destination_file \u003d rtslib.root.default_save_file"},{"line_number":186,"context_line":"            path_to_file \u003d os.path.dirname(destination_file)"}],"source_content_type":"text/x-python","patch_set":3,"id":"7ad8896b_a22ae62a","line":183,"in_reply_to":"7ad8896b_0e1a0b6f","updated":"2015-05-11 17:54:23.000000000","message":"Done","commit_id":"cf2377bc71fd5fc9f5ce74386800098e35c85482"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"081a0caaea880cd994667ff7d3e4d0d2d8e8b423","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            # If run manually from the command line without being root you will"},{"line_number":193,"context_line":"            # get an error, same as when creating and removing targets."},{"line_number":194,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":195,"context_line":"                os.makedirs(path_to_file)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"        rtsroot.save_to_file(destination_file)"},{"line_number":198,"context_line":"    except (OSError, IOError) as exc:"}],"source_content_type":"text/x-python","patch_set":3,"id":"9af37de9_6fee12ee","line":195,"updated":"2015-05-11 15:17:21.000000000","message":"It may not be required, but I think it may be good to be explicit and pass mode of 0755 here.","commit_id":"cf2377bc71fd5fc9f5ce74386800098e35c85482"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"210130ebb46dbce7a9acdadc83e03d7a0b4856e1","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            # If run manually from the command line without being root you will"},{"line_number":193,"context_line":"            # get an error, same as when creating and removing targets."},{"line_number":194,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":195,"context_line":"                os.makedirs(path_to_file)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"        rtsroot.save_to_file(destination_file)"},{"line_number":198,"context_line":"    except (OSError, IOError) as exc:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7ad8896b_82200a4b","line":195,"in_reply_to":"9af37de9_6fee12ee","updated":"2015-05-11 17:54:23.000000000","message":"Done","commit_id":"cf2377bc71fd5fc9f5ce74386800098e35c85482"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"552574cff14124075bb52b081819bccf4cbbb04b","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            # If run manually from the command line without being root you will"},{"line_number":193,"context_line":"            # get an error, same as when creating and removing targets."},{"line_number":194,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_f657b723","line":195,"updated":"2015-05-22 14:58:17.000000000","message":"Some security hardened linux sets 700 on default. I think it would be more appropriate to apply umask for folder mode.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"0a949d5f414178c4c99e8694f6e4424b22591f0b","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            # If run manually from the command line without being root you will"},{"line_number":193,"context_line":"            # get an error, same as when creating and removing targets."},{"line_number":194,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_0129c789","line":195,"in_reply_to":"7a016987_262e198c","updated":"2015-05-25 13:02:11.000000000","message":"Good!","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a6ea0da01713152ca308b1c8625213d75689b850","unresolved":false,"context_lines":[{"line_number":192,"context_line":"            # If run manually from the command line without being root you will"},{"line_number":193,"context_line":"            # get an error, same as when creating and removing targets."},{"line_number":194,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_262e198c","line":195,"in_reply_to":"7a016987_f657b723","updated":"2015-05-25 12:28:44.000000000","message":"umask is automatically masked out of this value by makedirs.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"552574cff14124075bb52b081819bccf4cbbb04b","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":199,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"},{"line_number":200,"context_line":"                           {\u0027default_path\u0027: path_to_file, \u0027exc\u0027: exc})"},{"line_number":201,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_563d6bdb","line":198,"updated":"2015-05-22 14:58:17.000000000","message":"This error may not actually be raised by targetcli. If you want to print specific error message for it, how about do a check on the path?","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"5052a7efa3746d7f2fa5792dbc7a5299dc9ab67c","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":199,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"},{"line_number":200,"context_line":"                           {\u0027default_path\u0027: path_to_file, \u0027exc\u0027: exc})"},{"line_number":201,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_77fad3be","line":198,"in_reply_to":"7a016987_177d78e7","updated":"2015-05-29 15:09:57.000000000","message":"Got your thinking. I\u0027m OK with it now.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a6ea0da01713152ca308b1c8625213d75689b850","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":199,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"},{"line_number":200,"context_line":"                           {\u0027default_path\u0027: path_to_file, \u0027exc\u0027: exc})"},{"line_number":201,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_a66fa9c2","line":198,"in_reply_to":"7a016987_563d6bdb","updated":"2015-05-25 12:28:44.000000000","message":"I don\u0027t follow what you mean, this is raised when we know we don\u0027t have the path and we failed to create it.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"3834b97bc913de4bda037fc09619d2a4eb712d00","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":199,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"},{"line_number":200,"context_line":"                           {\u0027default_path\u0027: path_to_file, \u0027exc\u0027: exc})"},{"line_number":201,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_177d78e7","line":198,"in_reply_to":"7a016987_72739de8","updated":"2015-05-26 08:47:19.000000000","message":"The reason why we say targetcli is not installed is because the default destination directory that is created upon its installation does not exist and we\u0027ve been asked to save configuration there (destination_file is None or \u0027\u0027).","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"ad83806fde47ad01a2a40ee4a613a2ff4493d314","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":199,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"},{"line_number":200,"context_line":"                           {\u0027default_path\u0027: path_to_file, \u0027exc\u0027: exc})"},{"line_number":201,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_72739de8","line":198,"in_reply_to":"7a016987_8ca28b4a","updated":"2015-05-26 08:14:53.000000000","message":"IMHO targetcli is not the only reason that may results in an OSError here. But the error message \u0027targetcli not installed ...\u0027 lock the root cause to targetcli.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"0a949d5f414178c4c99e8694f6e4424b22591f0b","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":199,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"},{"line_number":200,"context_line":"                           {\u0027default_path\u0027: path_to_file, \u0027exc\u0027: exc})"},{"line_number":201,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_c155eff9","line":198,"in_reply_to":"7a016987_a66fa9c2","updated":"2015-05-25 13:02:11.000000000","message":"OSError is a general error. The targetcli may raise it. But other things may raise it too. If so the error message would make user confused.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"675911506309f338b0ee8183078b8d816dcc361f","unresolved":false,"context_lines":[{"line_number":195,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"    except OSError as exc:"},{"line_number":198,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":199,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"},{"line_number":200,"context_line":"                           {\u0027default_path\u0027: path_to_file, \u0027exc\u0027: exc})"},{"line_number":201,"context_line":"    try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_8ca28b4a","line":198,"in_reply_to":"7a016987_c155eff9","updated":"2015-05-25 15:46:45.000000000","message":"At this point I don\u0027t even know what you are talking about!\n\nI\u0027m raising RtstoolError exception, not OSError. I\u0027m catching OSError and I include its string representation in raised exception.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"7f34f4017983cb9147e0e4ea6829221bf4c6c00b","unresolved":false,"context_lines":[{"line_number":211,"context_line":"            # get an error, same as when creating and removing targets."},{"line_number":212,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":213,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"    except OSError as exc:"},{"line_number":216,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":217,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa32b979_304f866c","line":214,"updated":"2015-06-17 19:06:53.000000000","message":"Please remove extraneous newline.","commit_id":"434bbdf4d047fb206709674a8f0f9141405a84d9"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"b0beae05b60a96a1c18649442ab3172bdda7fad8","unresolved":false,"context_lines":[{"line_number":211,"context_line":"            # get an error, same as when creating and removing targets."},{"line_number":212,"context_line":"            if not os.path.exists(path_to_file):"},{"line_number":213,"context_line":"                os.makedirs(path_to_file, 0o755)"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"    except OSError as exc:"},{"line_number":216,"context_line":"        raise RtstoolError(_(\u0027targetcli not installed and could not create \u0027"},{"line_number":217,"context_line":"                             \u0027default directory (%(default_path)s): %(exc)s\u0027),"}],"source_content_type":"text/x-python","patch_set":6,"id":"fa32b979_cb39cbd8","line":214,"in_reply_to":"fa32b979_304f866c","updated":"2015-06-17 19:28:24.000000000","message":"I actually did that on purpose, because with the long note it makes it easier to read the code, at least for me.\n\nI understand this is a matter of preferences, but since it\u0027s not against PEP-8, I\u0027d rather leave it as it is.","commit_id":"434bbdf4d047fb206709674a8f0f9141405a84d9"}],"cinder/tests/unit/test_cmd.py":[{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"552574cff14124075bb52b081819bccf4cbbb04b","unresolved":false,"context_lines":[{"line_number":946,"context_line":"        self.assertEqual(0, mock_os.makedirs.call_count)"},{"line_number":947,"context_line":""},{"line_number":948,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027os\u0027,"},{"line_number":949,"context_line":"                       **{\u0027path.exists.return_value\u0027: False,"},{"line_number":950,"context_line":"                          \u0027path.dirname.return_value\u0027: mock.sentinel.dirname})"},{"line_number":951,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027rtslib\u0027,"},{"line_number":952,"context_line":"                       **{\u0027root.default_save_file\u0027: mock.sentinel.filename})"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_a950c00b","line":949,"updated":"2015-05-22 14:58:17.000000000","message":"Can you test on a generated random name temp dir rather than set path.exists.return_value to be always False? Since this is a system function and may have very wide effects (maybe unexpected) if mocked.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"0a949d5f414178c4c99e8694f6e4424b22591f0b","unresolved":false,"context_lines":[{"line_number":946,"context_line":"        self.assertEqual(0, mock_os.makedirs.call_count)"},{"line_number":947,"context_line":""},{"line_number":948,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027os\u0027,"},{"line_number":949,"context_line":"                       **{\u0027path.exists.return_value\u0027: False,"},{"line_number":950,"context_line":"                          \u0027path.dirname.return_value\u0027: mock.sentinel.dirname})"},{"line_number":951,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027rtslib\u0027,"},{"line_number":952,"context_line":"                       **{\u0027root.default_save_file\u0027: mock.sentinel.filename})"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_a159d3fe","line":949,"in_reply_to":"7a016987_46c7d563","updated":"2015-05-25 13:02:11.000000000","message":"IMHO what your test case needs is a previously non-existing dir. You  can a temp one. There is no need to patch a system function (which affects more than you need). Days later when someone is editing the code here and failed to realize what is patched, it may introduce new bug.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":13900,"name":"Accela Zhao","email":"accelazh@gmail.com","username":"accelazh"},"change_message_id":"ad83806fde47ad01a2a40ee4a613a2ff4493d314","unresolved":false,"context_lines":[{"line_number":946,"context_line":"        self.assertEqual(0, mock_os.makedirs.call_count)"},{"line_number":947,"context_line":""},{"line_number":948,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027os\u0027,"},{"line_number":949,"context_line":"                       **{\u0027path.exists.return_value\u0027: False,"},{"line_number":950,"context_line":"                          \u0027path.dirname.return_value\u0027: mock.sentinel.dirname})"},{"line_number":951,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027rtslib\u0027,"},{"line_number":952,"context_line":"                       **{\u0027root.default_save_file\u0027: mock.sentinel.filename})"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_54c40e5e","line":949,"in_reply_to":"7a016987_8c472b77","updated":"2015-05-26 08:14:53.000000000","message":"I `grep -ir \u0027mock\\.patch.*os\\.path\\.exists\u0027 cinder` find many existing patching on os.path.exists. I still think the patch is too \u0027wide\u0027, but it is OK since there are already existing uses.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"675911506309f338b0ee8183078b8d816dcc361f","unresolved":false,"context_lines":[{"line_number":946,"context_line":"        self.assertEqual(0, mock_os.makedirs.call_count)"},{"line_number":947,"context_line":""},{"line_number":948,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027os\u0027,"},{"line_number":949,"context_line":"                       **{\u0027path.exists.return_value\u0027: False,"},{"line_number":950,"context_line":"                          \u0027path.dirname.return_value\u0027: mock.sentinel.dirname})"},{"line_number":951,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027rtslib\u0027,"},{"line_number":952,"context_line":"                       **{\u0027root.default_save_file\u0027: mock.sentinel.filename})"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_8c472b77","line":949,"in_reply_to":"7a016987_a159d3fe","updated":"2015-05-25 15:46:45.000000000","message":"Are you joking?\n\nI\u0027m patching exactly what I need patched to behave how I need it to behave during this test, and if someone tomorrow needs to modify the test I don\u0027t thing it\u0027s too hard to figure out that it\u0027s patching the method. After all the docstring talks about path not existing.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a6ea0da01713152ca308b1c8625213d75689b850","unresolved":false,"context_lines":[{"line_number":946,"context_line":"        self.assertEqual(0, mock_os.makedirs.call_count)"},{"line_number":947,"context_line":""},{"line_number":948,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027os\u0027,"},{"line_number":949,"context_line":"                       **{\u0027path.exists.return_value\u0027: False,"},{"line_number":950,"context_line":"                          \u0027path.dirname.return_value\u0027: mock.sentinel.dirname})"},{"line_number":951,"context_line":"    @mock.patch.object(cinder_rtstool, \u0027rtslib\u0027,"},{"line_number":952,"context_line":"                       **{\u0027root.default_save_file\u0027: mock.sentinel.filename})"}],"source_content_type":"text/x-python","patch_set":4,"id":"7a016987_46c7d563","line":949,"in_reply_to":"7a016987_a950c00b","updated":"2015-05-25 12:28:44.000000000","message":"I don\u0027t understand what you are talking about, \"wide effects\"? \"unexpected\"?\n\nWe are just mocking this for the duration of this specific test method and it will have the expected effect, it will return False when it is called simulating a non existing path.","commit_id":"2d2983b8073d643ba76927325abe3de3871bd7f7"}]}