This is an automated email from the ASF dual-hosted git repository. root pushed a commit to branch aevri/safe_noninteractive in repository https://gitbox.apache.org/repos/asf/buildstream.git
commit d7c8f739a7b4be102d6d8aac846f1e7b54048f03 Author: Angelos Evripiotis <[email protected]> AuthorDate: Fri Nov 16 14:48:17 2018 +0000 BREAK: make destructive action scripts consistent Make sure that BuildStream doesn't behave differently about destructive things just because it's in non-interactive mode, e.g. being run from a misconfigured workflow helper script. This is a breaking change, as the behaviour will not be the same for folks that have already scripted the use of these commands. By default, assume that users opt-out of destructive actions if we are unable to ask them. For the currently affected commands, this means aborting completely. This seems less surprising than doing the destructive action instead. Also, for folks that find the new behaviour surprising, at least they don't lose work. Introduce an '--assume-yes' option to affected commands to override this behaviour. If a command aborts for this reason, print a message suggesting the use of '--assume-yes'. Don't mention the corresponding 'prompt.*' options that also control this behaviour - this would hinder portability of scripts between users with different settings. This affects: - bst workspace close --remove-dir - bst workspace reset Closes #744, #726. --- NEWS | 9 +++++++++ buildstream/_frontend/cli.py | 30 ++++++++++++++++++++++-------- tests/examples/developing.py | 6 ++++-- tests/examples/junctions.py | 5 +++-- tests/frontend/cross_junction_workspace.py | 4 ++-- tests/frontend/workspace.py | 21 +++++++++++---------- tests/plugins/filter.py | 2 +- 7 files changed, 52 insertions(+), 25 deletions(-) diff --git a/NEWS b/NEWS index 16759e1..01569fa 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,12 @@ buildstream 1.3.1 make changes to their .bst files if they are expecting these environment variables to be set. + o BREAKING CHANGE: When non-interactive, `bst workspace` commands `reset` and + `close --remove-dir` will abort unless told not to do so. This is to avoid + surprises around destructive actions. The new `--assume-yes` option to + those commands is the preferred way to do this for scripts. The new + `prompt.really-workspace-*` options in buildstream.conf is another way. + o Failed builds are included in the cache as well. `bst checkout` will provide anything in `%{install-root}`. A build including cached fails will cause any dependant elements @@ -51,6 +57,9 @@ buildstream 1.3.1 certain confirmation prompts, e.g. double-checking that the user meant to run the command as typed. + o The `bst workspace` commands `close` and `reset` learned an `--assume-yes` + option, for supressing prompts. + o Due to the element `build tree` being cached in the respective artifact their size in some cases has significantly increased. In *most* cases the build trees are not utilised when building targets, as such by default bst 'pull' & 'build' diff --git a/buildstream/_frontend/cli.py b/buildstream/_frontend/cli.py index b1b4e03..8375de6 100644 --- a/buildstream/_frontend/cli.py +++ b/buildstream/_frontend/cli.py @@ -734,10 +734,12 @@ def workspace_open(app, no_checkout, force, track_, directory, elements): help="Remove the path that contains the closed workspace") @click.option('--all', '-a', 'all_', default=False, is_flag=True, help="Close all open workspaces") [email protected]('--assume-yes', '-y', default=False, is_flag=True, + help="Assume 'yes' to confirmation of destructive changes") @click.argument('elements', nargs=-1, type=click.Path(readable=False)) @click.pass_obj -def workspace_close(app, remove_dir, all_, elements): +def workspace_close(app, remove_dir, all_, assume_yes, elements): """Close a workspace""" if not (all_ or elements): @@ -764,9 +766,14 @@ def workspace_close(app, remove_dir, all_, elements): if nonexisting: raise AppError("Workspace does not exist", detail="\n".join(nonexisting)) - if app.interactive and remove_dir and app.context.prompt_workspace_close_remove_dir: - if not click.confirm('This will remove all your changes, are you sure?'): - click.echo('Aborting', err=True) + if remove_dir and not assume_yes and app.context.prompt_workspace_close_remove_dir: + if app.interactive: + if not click.confirm('This will remove all your changes, are you sure?'): + click.echo('Aborting', err=True) + sys.exit(-1) + else: + click.echo("Aborted destructive non-interactive action.", err=True) + click.echo("Please use the '--assume-yes' option to override.", err=True) sys.exit(-1) for element_name in elements: @@ -783,10 +790,12 @@ def workspace_close(app, remove_dir, all_, elements): help="Track and fetch the latest source before resetting") @click.option('--all', '-a', 'all_', default=False, is_flag=True, help="Reset all open workspaces") [email protected]('--assume-yes', '-y', default=False, is_flag=True, + help="Assume 'yes' to confirmation of destructive changes") @click.argument('elements', nargs=-1, type=click.Path(readable=False)) @click.pass_obj -def workspace_reset(app, soft, track_, all_, elements): +def workspace_reset(app, soft, track_, all_, assume_yes, elements): """Reset a workspace to its original state""" # Check that the workspaces in question exist @@ -798,9 +807,14 @@ def workspace_reset(app, soft, track_, all_, elements): if all_ and not app.stream.workspace_exists(): raise AppError("No open workspaces to reset") - if app.interactive and not soft and app.context.prompt_workspace_reset_hard: - if not click.confirm('This will remove all your changes, are you sure?'): - click.echo('Aborting', err=True) + if not soft and not assume_yes and app.context.prompt_workspace_reset_hard: + if app.interactive: + if not click.confirm('This will remove all your changes, are you sure?'): + click.echo('Aborting', err=True) + sys.exit(-1) + else: + click.echo("Aborted destructive non-interactive action.", err=True) + click.echo("Please use the '--assume-yes' option to override.", err=True) sys.exit(-1) if all_: diff --git a/tests/examples/developing.py b/tests/examples/developing.py index 4bb7076..7254794 100644 --- a/tests/examples/developing.py +++ b/tests/examples/developing.py @@ -65,7 +65,8 @@ def test_open_workspace(cli, tmpdir, datafiles): result = cli.run(project=project, args=['workspace', 'list']) result.assert_success() - result = cli.run(project=project, args=['workspace', 'close', '--remove-dir', 'hello.bst']) + result = cli.run( + project=project, args=['workspace', 'close', '--remove-dir', '--assume-yes', 'hello.bst']) result.assert_success() @@ -95,5 +96,6 @@ def test_make_change_in_workspace(cli, tmpdir, datafiles): result.assert_success() assert result.output == 'Hello World\nWe can use workspaces!\n' - result = cli.run(project=project, args=['workspace', 'close', '--remove-dir', 'hello.bst']) + result = cli.run( + project=project, args=['workspace', 'close', '--remove-dir', '--assume-yes', 'hello.bst']) result.assert_success() diff --git a/tests/examples/junctions.py b/tests/examples/junctions.py index 97c622b..ddcada1 100644 --- a/tests/examples/junctions.py +++ b/tests/examples/junctions.py @@ -51,6 +51,7 @@ def test_open_cross_junction_workspace(cli, tmpdir, datafiles): args=['workspace', 'open', '--directory', workspace_dir, 'hello-junction.bst:hello.bst']) result.assert_success() - result = cli.run(project=project, - args=['workspace', 'close', '--remove-dir', 'hello-junction.bst:hello.bst']) + result = cli.run( + project=project, + args=['workspace', 'close', '--remove-dir', '--assume-yes', 'hello-junction.bst:hello.bst']) result.assert_success() diff --git a/tests/frontend/cross_junction_workspace.py b/tests/frontend/cross_junction_workspace.py index fb2b34c..e265c9e 100644 --- a/tests/frontend/cross_junction_workspace.py +++ b/tests/frontend/cross_junction_workspace.py @@ -82,7 +82,7 @@ def test_close_cross_junction(cli, tmpdir): project, workspace = open_cross_junction(cli, tmpdir) element = 'sub.bst:data.bst' - args = ['workspace', 'close', '--remove-dir', element] + args = ['workspace', 'close', '--remove-dir', '--assume-yes', element] result = cli.run(project=project, args=args) result.assert_success() @@ -101,7 +101,7 @@ def test_close_cross_junction(cli, tmpdir): def test_close_all_cross_junction(cli, tmpdir): project, workspace = open_cross_junction(cli, tmpdir) - args = ['workspace', 'close', '--remove-dir', '--all'] + args = ['workspace', 'close', '--remove-dir', '--assume-yes', '--all'] result = cli.run(project=project, args=args) result.assert_success() diff --git a/tests/frontend/workspace.py b/tests/frontend/workspace.py index bc928ad..03ae262 100644 --- a/tests/frontend/workspace.py +++ b/tests/frontend/workspace.py @@ -393,7 +393,7 @@ def test_close(cli, tmpdir, datafiles, kind): # Close the workspace result = cli.run(project=project, args=[ - 'workspace', 'close', '--remove-dir', element_name + 'workspace', 'close', '--remove-dir', '--assume-yes', element_name ]) result.assert_success() @@ -413,7 +413,7 @@ def test_close_external_after_move_project(cli, tmpdir, datafiles): # Close the workspace result = cli.run(project=moved_dir, args=[ - 'workspace', 'close', '--remove-dir', element_name + 'workspace', 'close', '--remove-dir', '--assume-yes', element_name ]) result.assert_success() @@ -433,7 +433,7 @@ def test_close_internal_after_move_project(cli, tmpdir, datafiles): # Close the workspace result = cli.run(project=moved_dir, args=[ - 'workspace', 'close', '--remove-dir', element_name + 'workspace', 'close', '--remove-dir', '--assume-yes', element_name ]) result.assert_success() @@ -471,7 +471,7 @@ def test_close_nonexistant_element(cli, tmpdir, datafiles): # Close the workspace result = cli.run(project=project, args=[ - 'workspace', 'close', '--remove-dir', element_name + 'workspace', 'close', '--remove-dir', '--assume-yes', element_name ]) result.assert_success() @@ -490,7 +490,7 @@ def test_close_multiple(cli, tmpdir, datafiles): # Close the workspaces result = cli.run(project=project, args=[ - 'workspace', 'close', '--remove-dir', alpha, beta + 'workspace', 'close', '--remove-dir', '--assume-yes', alpha, beta ]) result.assert_success() @@ -510,7 +510,7 @@ def test_close_all(cli, tmpdir, datafiles): # Close the workspaces result = cli.run(project=project, args=[ - 'workspace', 'close', '--remove-dir', '--all' + 'workspace', 'close', '--remove-dir', '--assume-yes', '--all' ]) result.assert_success() @@ -533,7 +533,7 @@ def test_reset(cli, tmpdir, datafiles): # Now reset the open workspace, this should have the # effect of reverting our changes. result = cli.run(project=project, args=[ - 'workspace', 'reset', element_name + 'workspace', 'reset', '--assume-yes', element_name ]) result.assert_success() assert os.path.exists(os.path.join(workspace, 'usr', 'bin', 'hello')) @@ -559,7 +559,7 @@ def test_reset_multiple(cli, tmpdir, datafiles): # Now reset the open workspaces, this should have the # effect of reverting our changes. result = cli.run(project=project, args=[ - 'workspace', 'reset', alpha, beta, + 'workspace', 'reset', '--assume-yes', alpha, beta, ]) result.assert_success() assert os.path.exists(os.path.join(workspace_alpha, 'usr', 'bin', 'hello')) @@ -585,7 +585,7 @@ def test_reset_all(cli, tmpdir, datafiles): # Now reset the open workspace, this should have the # effect of reverting our changes. result = cli.run(project=project, args=[ - 'workspace', 'reset', '--all' + 'workspace', 'reset', '--assume-yes', '--all' ]) result.assert_success() assert os.path.exists(os.path.join(workspace_alpha, 'usr', 'bin', 'hello')) @@ -947,7 +947,8 @@ def test_list_supported_workspace(cli, tmpdir, datafiles, workspace_cfg, expecte # Make a change to the workspaces file result = cli.run(project=project, args=['workspace', 'open', '--directory', workspace, element_name]) result.assert_success() - result = cli.run(project=project, args=['workspace', 'close', '--remove-dir', element_name]) + result = cli.run( + project=project, args=['workspace', 'close', '--remove-dir', '--assume-yes', element_name]) result.assert_success() # Check that workspace config is converted correctly if necessary diff --git a/tests/plugins/filter.py b/tests/plugins/filter.py index 6dae085..0a7fa1e 100644 --- a/tests/plugins/filter.py +++ b/tests/plugins/filter.py @@ -172,7 +172,7 @@ def test_filter_workspace_reset(datafiles, cli, tmpdir): src = os.path.join(workspace_dir, "foo") dst = os.path.join(workspace_dir, "quux") shutil.copyfile(src, dst) - result = cli.run(project=project, args=['workspace', 'reset', 'deps-permitted.bst']) + result = cli.run(project=project, args=['workspace', 'reset', '--assume-yes', 'deps-permitted.bst']) result.assert_success() result = cli.run(project=project, args=['build', 'output-orphans.bst']) result.assert_success()
