From ab7ba1a6e700deaa183c4bfd05cd2797eac5b946 Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Mon, 2 Feb 2015 14:04:41 -0800 Subject: [PATCH] Remove session dep from CommandParameters CommandParameters is moving (but is not quite there) towards being only concerned with command parameter validation and restructuring. It's should not be concerned with also making S3 requests, _especially_ recursively deleting customer objects. This is better suited for the CommandArchitecture or the RbCommand objects. While it would take more work to get it there, the S3TransferCommand base class seems to be a better fit and an incremental improvement. It makes the CommandParameters class simpler. --- awscli/customizations/s3/subcommands.py | 47 +++++++++++-------- .../customizations/s3/test_subcommands.py | 27 ++++------- 2 files changed, 36 insertions(+), 38 deletions(-) diff --git a/awscli/customizations/s3/subcommands.py b/awscli/customizations/s3/subcommands.py index 10aec665efa9..eadc29d23af2 100644 --- a/awscli/customizations/s3/subcommands.py +++ b/awscli/customizations/s3/subcommands.py @@ -460,14 +460,14 @@ def _run_main(self, parsed_args, parsed_globals): super(S3TransferCommand, self)._run_main(parsed_args, parsed_globals) self._convert_path_args(parsed_args) params = self._build_call_parameters(parsed_args, {}) - cmd_params = CommandParameters(self._session, self.NAME, params, + cmd_params = CommandParameters(self.NAME, params, self.USAGE) cmd_params.add_region(parsed_globals) cmd_params.add_endpoint_url(parsed_globals) cmd_params.add_verify_ssl(parsed_globals) cmd_params.add_page_size(parsed_args) cmd_params.add_paths(parsed_args.paths) - cmd_params.check_force(parsed_globals) + self._handle_rm_force(parsed_globals, cmd_params.parameters) cmd = CommandArchitecture(self._session, self.NAME, cmd_params.parameters) cmd.set_endpoints() @@ -494,6 +494,26 @@ def _convert_path_args(self, parsed_args): new_path = enc_path.decode('utf-8') parsed_args.paths[i] = new_path + def _handle_rm_force(self, parsed_globals, parameters): + """ + This function recursive deletes objects in a bucket if the force + parameters was thrown when using the remove bucket command. + """ + # XXX: This shouldn't really be here. This was originally moved from + # the CommandParameters class to here, but this is still not the ideal + # place for this code. This should be moved + # to either the CommandArchitecture class, or the RbCommand class where + # the actual operations against S3 are performed. This may require + # some refactoring though to move this to either of those classes. + # For now, moving this out of CommandParameters allows for that class + # to be kept simple. + if 'force' in parameters: + if parameters['force']: + bucket = find_bucket_key(parameters['src'][5:])[0] + path = 's3://' + bucket + del_objects = RmCommand(self._session) + del_objects([path, '--recursive'], parsed_globals) + class CpCommand(S3TransferCommand): NAME = 'cp' @@ -789,12 +809,16 @@ class CommandParameters(object): This class is used to do some initial error based on the parameters and arguments passed to the command line. """ - def __init__(self, session, cmd, parameters, usage): + def __init__(self, cmd, parameters, usage): """ Stores command name and parameters. Ensures that the ``dir_op`` flag is true if a certain command is being used. + + :param cmd: The name of the command, e.g. "rm". + :param parameters: A dictionary of parameters. + :param usage: A usage string + """ - self.session = session self.cmd = cmd self.parameters = parameters self.usage = usage @@ -914,21 +938,6 @@ def check_src_path(self, paths): else: raise Exception("Error: Local path does not exist") - def check_force(self, parsed_globals): - """ - This function recursive deletes objects in a bucket if the force - parameters was thrown when using the remove bucket command. - """ - if 'force' in self.parameters: - if self.parameters['force']: - bucket = find_bucket_key(self.parameters['src'][5:])[0] - path = 's3://' + bucket - try: - del_objects = RmCommand(self.session) - del_objects([path, '--recursive'], parsed_globals) - except: - pass - def add_region(self, parsed_globals): self.parameters['region'] = parsed_globals.region diff --git a/tests/unit/customizations/s3/test_subcommands.py b/tests/unit/customizations/s3/test_subcommands.py index f99ab2504e07..a23682c050b0 100644 --- a/tests/unit/customizations/s3/test_subcommands.py +++ b/tests/unit/customizations/s3/test_subcommands.py @@ -447,16 +447,14 @@ def setUp(self): self.environ = {} self.environ_patch = patch('os.environ', self.environ) self.environ_patch.start() - self.session = FakeSession() self.mock = MagicMock() self.mock.get_config = MagicMock(return_value={'region': None}) self.loc_files = make_loc_files() - self.bucket = make_s3_files(self.session) + self.bucket = 's3testbucket' def tearDown(self): self.environ_patch.stop() clean_loc_files(self.loc_files) - s3_cleanup(self.bucket, self.session) def test_check_path_type_pass(self): # This tests the class's ability to determine whether the correct @@ -477,7 +475,7 @@ def test_check_path_type_pass(self): 'locallocal': [local_file, local_file]} for cmd in cmds.keys(): - cmd_param = CommandParameters(self.session, cmd, {}, '') + cmd_param = CommandParameters(cmd, {}, '') cmd_param.add_region(mock.Mock()) correct_paths = cmds[cmd] for path_args in correct_paths: @@ -505,7 +503,7 @@ def test_check_path_type_fail(self): 'locallocal': [local_file, local_file]} for cmd in cmds.keys(): - cmd_param = CommandParameters(self.session, cmd, {}, '') + cmd_param = CommandParameters(cmd, {}, '') cmd_param.add_region(mock.Mock()) wrong_paths = cmds[cmd] for path_args in wrong_paths: @@ -531,22 +529,13 @@ def test_check_src_path_pass(self): parameters = {} for filename in files: parameters['dir_op'] = filename[1] - cmd_parameter = CommandParameters(self.session, 'put', - parameters, '') + cmd_parameter = CommandParameters('put', parameters, '') cmd_parameter.add_region(mock.Mock()) cmd_parameter.check_src_path(filename[0]) - def test_check_force(self): - # This checks to make sure that the force parameter is run. If - # successful. The delete command will fail as the bucket is empty - # and be caught by the exception. - cmd_params = CommandParameters(self.session, 'rb', {'force': True},'') - cmd_params.parameters['src'] = 's3://mybucket' - cmd_params.check_force(None) - def test_validate_streaming_paths_upload(self): parameters = {'src': '-', 'dest': 's3://bucket'} - cmd_params = CommandParameters(self.session, 'cp', parameters, '') + cmd_params = CommandParameters('cp', parameters, '') cmd_params._validate_streaming_paths() self.assertTrue(cmd_params.parameters['is_stream']) self.assertTrue(cmd_params.parameters['only_show_errors']) @@ -554,7 +543,7 @@ def test_validate_streaming_paths_upload(self): def test_validate_streaming_paths_download(self): parameters = {'src': 'localfile', 'dest': '-'} - cmd_params = CommandParameters(self.session, 'cp', parameters, '') + cmd_params = CommandParameters('cp', parameters, '') cmd_params._validate_streaming_paths() self.assertTrue(cmd_params.parameters['is_stream']) self.assertTrue(cmd_params.parameters['only_show_errors']) @@ -562,13 +551,13 @@ def test_validate_streaming_paths_download(self): def test_validate_no_streaming_paths(self): parameters = {'src': 'localfile', 'dest': 's3://bucket'} - cmd_params = CommandParameters(self.session, 'cp', parameters, '') + cmd_params = CommandParameters('cp', parameters, '') cmd_params._validate_streaming_paths() self.assertFalse(cmd_params.parameters['is_stream']) def test_validate_streaming_paths_error(self): parameters = {'src': '-', 'dest': 's3://bucket'} - cmd_params = CommandParameters(self.session, 'sync', parameters, '') + cmd_params = CommandParameters('sync', parameters, '') with self.assertRaises(ValueError): cmd_params._validate_streaming_paths()