diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a5dc9b8a35fd..c248e8da92ef 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,12 @@ CHANGELOG ========= +Next Release (TBD) +================== +* bugfix:``aws s3``: Fix some local path validation issues + (`issue 1575 `__) + + 1.9.1 ===== * feature:``aws ssm``: Add support for Amazon EC2 Run Command diff --git a/awscli/customizations/s3/subcommands.py b/awscli/customizations/s3/subcommands.py index d08abe0e9dc0..d3acb34a3dc1 100644 --- a/awscli/customizations/s3/subcommands.py +++ b/awscli/customizations/s3/subcommands.py @@ -914,6 +914,20 @@ def _validate_path_args(self): raise ValueError("Cannot mv a file onto itself: '%s' - '%s'" % ( params['src'], params['dest'])) + # If the user provided local path does not exist, hard fail because + # we know that we will not be able to upload the file. + if 'locals3' == params['paths_type'] and not params['is_stream']: + if not os.path.exists(params['src']): + raise RuntimeError( + 'The user-provided path %s does not exist.' % + params['src']) + # If the operation is downloading to a directory that does not exist, + # create the directories so no warnings are thrown during the syncing + # process. + elif 's3local' == params['paths_type'] and params['dir_op']: + if not os.path.exists(params['dest']): + os.makedirs(params['dest']) + def _same_path(self, src, dest): if not self.parameters['paths_type'] == 's3s3': return False @@ -957,35 +971,6 @@ def check_path_type(self, paths): else: raise TypeError("%s\nError: Invalid argument type" % usage) - def check_src_path(self, paths): - """ - This checks the source paths to deem if they are valid. The check - performed in S3 is first it lists the objects using the source path. - If there is an error like the bucket does not exist, the error will be - caught with ``check_error()`` function. If the operation is on a - single object in s3, it checks that a list of object was returned and - that the first object listed is the name of the specified in the - command line. If the operation is on objects under a common prefix, - it will check that there are common prefixes and objects under - the specified prefix. - For local files, it first checks that the path exists. Then it checks - that the path is a directory if it is a directory operation or that - the path is a file if the operation is on a single file. - """ - src_path = paths[0] - dir_op = self.parameters['dir_op'] - if not src_path.startswith('s3://'): - src_path = os.path.abspath(src_path) - if os.path.exists(src_path): - if os.path.isdir(src_path) and not dir_op: - raise Exception("Error: Requires a local file") - elif os.path.isfile(src_path) and dir_op: - raise Exception("Error: Requires a local directory") - else: - pass - else: - raise Exception("Error: Local path does not exist") - def add_region(self, parsed_globals): self.parameters['region'] = parsed_globals.region diff --git a/tests/functional/s3/test_sync_command.py b/tests/functional/s3/test_sync_command.py index d4c92200ded5..40d88a2569cb 100644 --- a/tests/functional/s3/test_sync_command.py +++ b/tests/functional/s3/test_sync_command.py @@ -11,9 +11,8 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. from awscli.testutils import BaseAWSCommandParamsTest, FileCreator -import re +import os -import mock from awscli.compat import six @@ -54,3 +53,28 @@ def test_no_recursive_option(self): cmdline = '. s3://mybucket --recursive' # Return code will be 2 for invalid parameter ``--recursive`` self.run_cmd(cmdline, expected_rc=2) + + def test_sync_from_non_existant_directory(self): + non_existant_directory = os.path.join(self.files.rootdir, 'fakedir') + cmdline = '%s %s s3://bucket/' % (self.prefix, non_existant_directory) + self.parsed_responses = [ + {"CommonPrefixes": [], "Contents": []} + ] + _, stderr, _ = self.run_cmd(cmdline, expected_rc=255) + self.assertIn('does not exist', stderr) + + def test_sync_to_non_existant_directory(self): + key = 'foo.txt' + non_existant_directory = os.path.join(self.files.rootdir, 'fakedir') + cmdline = '%s s3://bucket/ %s' % (self.prefix, non_existant_directory) + self.parsed_responses = [ + {"CommonPrefixes": [], "Contents": [ + {"Key": key, "Size": 3, + "LastModified": "2014-01-09T20:45:49.000Z"}]}, + {'ETag': '"c8afdb36c52cf4727836669019e69222-"', + 'Body': six.BytesIO(b'foo')} + ] + self.run_cmd(cmdline, expected_rc=0) + # Make sure the file now exists. + self.assertTrue( + os.path.exists(os.path.join(non_existant_directory, key))) diff --git a/tests/integration/customizations/s3/test_plugin.py b/tests/integration/customizations/s3/test_plugin.py index 520a0313662b..0b3bad3f8966 100644 --- a/tests/integration/customizations/s3/test_plugin.py +++ b/tests/integration/customizations/s3/test_plugin.py @@ -778,8 +778,10 @@ def extra_setup(self): def test_no_exist(self): filename = os.path.join(self.files.rootdir, "no-exists-file") p = aws('s3 cp %s s3://%s/' % (filename, self.bucket_name)) - self.assertEqual(p.rc, 2, p.stderr) - self.assertIn('warning: Skipping file %s. File does not exist.' % + # If the local path provided by the user is nonexistant for an + # upload, this should error out. + self.assertEqual(p.rc, 255, p.stderr) + self.assertIn('The user-provided path %s does not exist.' % filename, p.stderr) @unittest.skipIf(platform.system() not in ['Darwin', 'Linux'], diff --git a/tests/unit/customizations/s3/test_subcommands.py b/tests/unit/customizations/s3/test_subcommands.py index e46eb086bf14..168834472071 100644 --- a/tests/unit/customizations/s3/test_subcommands.py +++ b/tests/unit/customizations/s3/test_subcommands.py @@ -596,49 +596,26 @@ def test_check_path_type_fail(self): with self.assertRaises(TypeError): cmd_param.check_path_type(combos[path_args]) - def test_check_src_path_pass(self): - # This tests to see if all of the checks on the source path works. It - # does so by testing if s3 objects and and prefixes exist as well as - # local files and directories. All of these should not throw an - # exception. - s3_file = 's3://' + self.bucket + '/' + 'text1.txt' - local_file = self.loc_files[0] - s3_prefix = 's3://' + self.bucket - local_dir = self.loc_files[3] - - # :var files: a list of tuples where the first element is a single - # element list of file paths. The second element is a boolean - # representing if the operation is a directory operation. - files = [([s3_file], False), ([local_file], False), - ([s3_prefix], True), ([local_dir], True)] - - parameters = {} - for filename in files: - parameters['dir_op'] = filename[1] - cmd_parameter = CommandParameters('put', parameters, '') - cmd_parameter.add_region(mock.Mock()) - cmd_parameter.check_src_path(filename[0]) - def test_validate_streaming_paths_upload(self): - parameters = {'src': '-', 'dest': 's3://bucket'} - cmd_params = CommandParameters('cp', parameters, '') - cmd_params._validate_streaming_paths() + paths = ['-', 's3://bucket'] + cmd_params = CommandParameters('cp', {}, '') + cmd_params.add_paths(paths) self.assertTrue(cmd_params.parameters['is_stream']) self.assertTrue(cmd_params.parameters['only_show_errors']) self.assertFalse(cmd_params.parameters['dir_op']) def test_validate_streaming_paths_download(self): - parameters = {'src': 'localfile', 'dest': '-'} - cmd_params = CommandParameters('cp', parameters, '') - cmd_params._validate_streaming_paths() + paths = ['s3://bucket/key', '-'] + cmd_params = CommandParameters('cp', {}, '') + cmd_params.add_paths(paths) self.assertTrue(cmd_params.parameters['is_stream']) self.assertTrue(cmd_params.parameters['only_show_errors']) self.assertFalse(cmd_params.parameters['dir_op']) def test_validate_no_streaming_paths(self): - parameters = {'src': 'localfile', 'dest': 's3://bucket'} - cmd_params = CommandParameters('cp', parameters, '') - cmd_params._validate_streaming_paths() + paths = [self.file_creator.rootdir, 's3://bucket'] + cmd_params = CommandParameters('cp', {}, '') + cmd_params.add_paths(paths) self.assertFalse(cmd_params.parameters['is_stream']) def test_validate_streaming_paths_error(self): @@ -647,6 +624,20 @@ def test_validate_streaming_paths_error(self): with self.assertRaises(ValueError): cmd_params._validate_streaming_paths() + def test_validate_non_existent_local_path_upload(self): + non_existent_path = os.path.join(self.file_creator.rootdir, 'foo') + paths = [non_existent_path, 's3://bucket/'] + cmd_param = CommandParameters('cp', {}, '') + with self.assertRaises(RuntimeError): + cmd_param.add_paths(paths) + + def test_add_path_for_non_existsent_local_path_download(self): + non_existent_path = os.path.join(self.file_creator.rootdir, 'foo') + paths = ['s3://bucket', non_existent_path] + cmd_param = CommandParameters('cp', {'dir_op': True}, '') + cmd_param.add_paths(paths) + self.assertTrue(os.path.exists(non_existent_path)) + class HelpDocTest(BaseAWSHelpOutputTest): def setUp(self):