)]}'
{"diskimage_builder/block_device/utils.py":[{"author":{"_account_id":23630,"name":"Tovin Seven","email":"tovin07@gmail.com","username":"tovin07"},"change_message_id":"fda0c1fafa1f6f22ce68f95a8375e953bf3fad79","unresolved":false,"context_lines":[{"line_number":93,"context_line":"    Run command under sudo, with debug trace of output."},{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    Arguments:"},{"line_number":96,"context_line":"    :param cmd: str command list; for Popen()"},{"line_number":97,"context_line":"    \"\"\""},{"line_number":98,"context_line":"    sudo_cmd \u003d [\"sudo\"]"},{"line_number":99,"context_line":"    sudo_cmd.extend(cmd)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f044301_ad98bfbf","line":96,"updated":"2017-05-09 06:43:35.000000000","message":"We have ``param``, adding ``return`` will make more sense :D","commit_id":"ccab9b1fac3ad0079f3f710d9c2dd3eb06abda50"},{"author":{"_account_id":23630,"name":"Tovin Seven","email":"tovin07@gmail.com","username":"tovin07"},"change_message_id":"fda0c1fafa1f6f22ce68f95a8375e953bf3fad79","unresolved":false,"context_lines":[{"line_number":96,"context_line":"    :param cmd: str command list; for Popen()"},{"line_number":97,"context_line":"    \"\"\""},{"line_number":98,"context_line":"    sudo_cmd \u003d [\"sudo\"]"},{"line_number":99,"context_line":"    sudo_cmd.extend(cmd)"},{"line_number":100,"context_line":"    try:"},{"line_number":101,"context_line":"        logger.info(\"Calling [%s]\" % \u0027 \u0027.join(sudo_cmd))"},{"line_number":102,"context_line":"    except TypeError:"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f044301_8dd8637f","line":99,"range":{"start_line":99,"start_character":13,"end_line":99,"end_character":24},"updated":"2017-05-09 06:43:35.000000000","message":"Should we catch an exception here too? when cmd is not a list?","commit_id":"ccab9b1fac3ad0079f3f710d9c2dd3eb06abda50"},{"author":{"_account_id":23630,"name":"Tovin Seven","email":"tovin07@gmail.com","username":"tovin07"},"change_message_id":"fda0c1fafa1f6f22ce68f95a8375e953bf3fad79","unresolved":false,"context_lines":[{"line_number":98,"context_line":"    sudo_cmd \u003d [\"sudo\"]"},{"line_number":99,"context_line":"    sudo_cmd.extend(cmd)"},{"line_number":100,"context_line":"    try:"},{"line_number":101,"context_line":"        logger.info(\"Calling [%s]\" % \u0027 \u0027.join(sudo_cmd))"},{"line_number":102,"context_line":"    except TypeError:"},{"line_number":103,"context_line":"        logger.exception(\"Ensure all arguments are str() type!\")"},{"line_number":104,"context_line":"        raise"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f044301_8d93c3a3","line":101,"range":{"start_line":101,"start_character":37,"end_line":101,"end_character":40},"updated":"2017-05-09 06:43:35.000000000","message":"double quote for consistent","commit_id":"ccab9b1fac3ad0079f3f710d9c2dd3eb06abda50"},{"author":{"_account_id":23630,"name":"Tovin Seven","email":"tovin07@gmail.com","username":"tovin07"},"change_message_id":"fda0c1fafa1f6f22ce68f95a8375e953bf3fad79","unresolved":false,"context_lines":[{"line_number":100,"context_line":"    try:"},{"line_number":101,"context_line":"        logger.info(\"Calling [%s]\" % \u0027 \u0027.join(sudo_cmd))"},{"line_number":102,"context_line":"    except TypeError:"},{"line_number":103,"context_line":"        logger.exception(\"Ensure all arguments are str() type!\")"},{"line_number":104,"context_line":"        raise"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"    proc \u003d subprocess.Popen(sudo_cmd,"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f044301_6d5517c6","line":103,"range":{"start_line":103,"start_character":51,"end_line":103,"end_character":54},"updated":"2017-05-09 06:43:35.000000000","message":"besides, does it work with unicode string?","commit_id":"ccab9b1fac3ad0079f3f710d9c2dd3eb06abda50"},{"author":{"_account_id":23630,"name":"Tovin Seven","email":"tovin07@gmail.com","username":"tovin07"},"change_message_id":"fda0c1fafa1f6f22ce68f95a8375e953bf3fad79","unresolved":false,"context_lines":[{"line_number":100,"context_line":"    try:"},{"line_number":101,"context_line":"        logger.info(\"Calling [%s]\" % \u0027 \u0027.join(sudo_cmd))"},{"line_number":102,"context_line":"    except TypeError:"},{"line_number":103,"context_line":"        logger.exception(\"Ensure all arguments are str() type!\")"},{"line_number":104,"context_line":"        raise"},{"line_number":105,"context_line":""},{"line_number":106,"context_line":"    proc \u003d subprocess.Popen(sudo_cmd,"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f044301_0d9fb3b4","line":103,"range":{"start_line":103,"start_character":54,"end_line":103,"end_character":56},"updated":"2017-05-09 06:43:35.000000000","message":"nit, just str","commit_id":"ccab9b1fac3ad0079f3f710d9c2dd3eb06abda50"},{"author":{"_account_id":23630,"name":"Tovin Seven","email":"tovin07@gmail.com","username":"tovin07"},"change_message_id":"fda0c1fafa1f6f22ce68f95a8375e953bf3fad79","unresolved":false,"context_lines":[{"line_number":107,"context_line":"                            stdout\u003dsubprocess.PIPE,"},{"line_number":108,"context_line":"                            stderr\u003dsubprocess.STDOUT)"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":"    for line in iter(proc.stdout.readline, b\u0027\u0027):"},{"line_number":111,"context_line":"        logger.debug(\"exec_sudo: %s\" % (line.rstrip(),))"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"    return proc.wait()"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f044301_6db9972c","line":110,"range":{"start_line":110,"start_character":44,"end_line":110,"end_character":46},"updated":"2017-05-09 06:43:35.000000000","message":"use double quote for consistent","commit_id":"ccab9b1fac3ad0079f3f710d9c2dd3eb06abda50"},{"author":{"_account_id":23630,"name":"Tovin Seven","email":"tovin07@gmail.com","username":"tovin07"},"change_message_id":"fda0c1fafa1f6f22ce68f95a8375e953bf3fad79","unresolved":false,"context_lines":[{"line_number":108,"context_line":"                            stderr\u003dsubprocess.STDOUT)"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":"    for line in iter(proc.stdout.readline, b\u0027\u0027):"},{"line_number":111,"context_line":"        logger.debug(\"exec_sudo: %s\" % (line.rstrip(),))"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"    return proc.wait()"},{"line_number":114,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3f044301_2d382f9e","line":111,"range":{"start_line":111,"start_character":39,"end_line":111,"end_character":55},"updated":"2017-05-09 06:43:35.000000000","message":"may be just::\n    logger.debug(\"exec_sudo: %s\" % line.rstrip())","commit_id":"ccab9b1fac3ad0079f3f710d9c2dd3eb06abda50"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"ab2bbe683db8c600d043e7020f1f484a864b2ec5","unresolved":false,"context_lines":[{"line_number":94,"context_line":""},{"line_number":95,"context_line":"    Arguments:"},{"line_number":96,"context_line":"    :param cmd: str command list; for Popen()"},{"line_number":97,"context_line":"    :return: int return code of the sudo process"},{"line_number":98,"context_line":"    \"\"\""},{"line_number":99,"context_line":"    assert isinstance(cmd, list)"},{"line_number":100,"context_line":"    sudo_cmd \u003d [\"sudo\"]"}],"source_content_type":"text/x-python","patch_set":10,"id":"3f044301_79e27503","line":97,"updated":"2017-05-09 11:23:26.000000000","message":"nit, but otherwise LGTM\n\n:return: 0 on success\n:raises: subprocess.CalledProcessError if return code !\u003d 0","commit_id":"52233d42b2fb8b61e08e7dcd56419f8531fea229"},{"author":{"_account_id":21741,"name":"Andreas Florath","email":"andreas@florath.net","username":"florath"},"change_message_id":"ac1349f290de9b0ca5da4eb8219e86867e7654b5","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        logger.debug(\"exec_sudo: %s\" % line.rstrip())"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"    proc.wait()"},{"line_number":117,"context_line":"    if proc.returncode !\u003d 0:"},{"line_number":118,"context_line":"        raise subprocess.CalledProcessError(proc.returncode,"},{"line_number":119,"context_line":"                                            \u0027 \u0027.join(sudo_cmd))"},{"line_number":120,"context_line":"    return 0"}],"source_content_type":"text/x-python","patch_set":11,"id":"3f044301_4a50f65d","line":117,"updated":"2017-05-09 17:25:05.000000000","message":"This behavior is \u0027unusual\u0027: either return True/False (0/1) or throw an exception when it fails and return None if ok.\nThis mixture is hard to handle.\n\nWhy not stay with the current interface? Later patches (like level3/mount) use the return value.","commit_id":"9d6048dc7dd3c5a65002574b58e453aa2626a50e"},{"author":{"_account_id":6133,"name":"yolanda.robla","email":"yroblamo@redhat.com","username":"yolanda.robla"},"change_message_id":"ea1b0533882571090e9efeb50d24d52f4c6f957b","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        logger.debug(\"exec_sudo: %s\" % line.rstrip())"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"    proc.wait()"},{"line_number":117,"context_line":"    if proc.returncode !\u003d 0:"},{"line_number":118,"context_line":"        raise subprocess.CalledProcessError(proc.returncode,"},{"line_number":119,"context_line":"                                            \u0027 \u0027.join(sudo_cmd))"},{"line_number":120,"context_line":"    return 0"}],"source_content_type":"text/x-python","patch_set":11,"id":"3f044301_e0e001cf","line":117,"in_reply_to":"3f044301_4a50f65d","updated":"2017-05-09 19:05:26.000000000","message":"So Ian suggested that behaviour to simulate what check_output is doing, throwing an exception if the return code is not valid. However, i see your point about not returning zero, it\u0027s an obvious value that does not provide any benefit.\nIan, Andreas, hy not emulate check_output 100% and return the stdout of the executed sudo command , or exception if that fails?","commit_id":"9d6048dc7dd3c5a65002574b58e453aa2626a50e"},{"author":{"_account_id":21741,"name":"Andreas Florath","email":"andreas@florath.net","username":"florath"},"change_message_id":"4e154b930c1cf89e5602eb891ce500133afeadb2","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        logger.debug(\"exec_sudo: %s\" % line.rstrip())"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"    proc.wait()"},{"line_number":117,"context_line":"    if proc.returncode !\u003d 0:"},{"line_number":118,"context_line":"        raise subprocess.CalledProcessError(proc.returncode,"},{"line_number":119,"context_line":"                                            \u0027 \u0027.join(sudo_cmd))"},{"line_number":120,"context_line":"    return 0"}],"source_content_type":"text/x-python","patch_set":11,"id":"3f044301_a65c4aed","line":117,"in_reply_to":"3f044301_b6411f0e","updated":"2017-05-10 09:36:46.000000000","message":"I\u0027m fine with the debug output: this is always very helpful.\n\n\u003e the callers seemed to be just doing \"if !\u003d 0 raise\" which they don\u0027t need to do any more; an exception is raised already.  So, less code. \n\nNot always ;-)\n\nThere are some places (e.g. diskimage_builder/block_device/level1/partitioning.py) where it is currently assumed that a command can possible fail.  So when changing the interface here we have to check all the callers and add some (ugly?): \"try: ... except ....: pass\" code around.\n\nWhich means: merging this patch - without adapting all the callers - introduces bugs.","commit_id":"9d6048dc7dd3c5a65002574b58e453aa2626a50e"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"9180ff53f70e400b11e7bb2d523599a1b5bad3bf","unresolved":false,"context_lines":[{"line_number":114,"context_line":"        logger.debug(\"exec_sudo: %s\" % line.rstrip())"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"    proc.wait()"},{"line_number":117,"context_line":"    if proc.returncode !\u003d 0:"},{"line_number":118,"context_line":"        raise subprocess.CalledProcessError(proc.returncode,"},{"line_number":119,"context_line":"                                            \u0027 \u0027.join(sudo_cmd))"},{"line_number":120,"context_line":"    return 0"}],"source_content_type":"text/x-python","patch_set":11,"id":"3f044301_b6411f0e","line":117,"in_reply_to":"3f044301_e0e001cf","updated":"2017-05-09 22:02:43.000000000","message":"sorry, I mis-typed; it\u0027s not check_output -- it\u0027s the other standard call \"check_call\" that works like this. \n\n@Andreas the callers seemed to be just doing \"if !\u003d 0 raise\" which they don\u0027t need to do any more; an exception is raised already.  So, less code. \n\ni want the tracing in here at log debug level, though.  Callers shouldn\u0027t have to handle that, and it\u0027s proven too valuable to us over time.  so it\u0027s not a straight wrapper.\n\nif we start passing back the stdout, we have to make decisions about also capturing stderror or adding **kwargs to pass through for redirect, and handling that with debug logging etc.  *if* we need that, we can consider it later -- it won\u0027t matter because existing callers just ignore any return value.","commit_id":"9d6048dc7dd3c5a65002574b58e453aa2626a50e"}]}