Added a `--source-region` parameter. #872

kyleknap · 2014-08-11T18:43:52Z

This parameter ensures the ability to do
trans-region syncs, moves, and copies. Tests
were expanded as well to better test
handling endpoints.

This parameter ensures the ability to do trans-region syncs, moves, and copies. Tests were expanded as well to better test handling endpoints.

kyleknap · 2014-08-11T18:56:27Z

This is a feature addition for cp, mv, and sync. So, if you want to sync two buckets that exist in different regions and avoid redirect errors, you would now enter aws s3 sync s3://us-west-1-bucket s3://ap-northeast-1-bucket --region ap-northeast-1 --source-region us-west-1 assuming the default region is not ap-northeast-1 region. The previous behavior used the region specified by the default region or --region for operations on both buckets. However, this resulted in 301 redirect errors if the bucket names were DNS incompatible. If --source-region is not specified the previous default behavior is used.

Tests were improved in two ways.
First, I added the ability to create buckets in different regions and the ability to name the bucket. The test remembers the region of the bucket using a dictionary to be able to do operations on objects in the bucket without having to specify the region.
Second, whenever local files are created, I subtract a few years off the the last modified time. This ensures local files are always older than uploaded s3 files.

kyleknap · 2014-08-11T18:58:36Z

@danielgtaylor @jamesls

jamesls · 2014-08-11T19:46:16Z

Can you clarify why the FileGenerator now needs both an endpoint and source_endpoint? I was expecting that for sync, one file generator gets the source_endpoint, and the other generator gets the destination endpoint, but the FileGenerator itself only needs a single endpoint argument.

I realize that this may be a slightly larger refactoring to do this, but I think it makes the most sense going forward.

kyleknap · 2014-08-11T20:22:05Z

FileGenerator for cp needs both the endpoint and source endpoint because the source_endpoint is needed to list out the objects in the source bucket, and the endpoint refers to the destination and that is needed for the actaul CopyObject command. When I was trying to figure out how to approach this, I learned for copying objects over there were two common redirects errors I got: One for ListObjects (which was thrown if I only used endpoint) and one for CopyObject (which was thrown if I only used source_endpoint). So I had to use both endpoints.

kyleknap · 2014-08-11T20:31:02Z

Also, this scenario is not limited to cp command. The same applies to sync and mv. In the case for mv, the source endpoint is needed for the DeleteObject operation. If the source endpoint is not used, you get a redirect error for DeleteObject

jamesls · 2014-08-11T22:30:19Z

What I was alluding to was that the FileGenerator class ideally shouldn't know about source/dest region. It's really just about listing local or remote files, for which the BucketLister only needs to know what region it's suppose to talk to . The only reason it needs both a source and a destination region is because it has to pass this info along to FileInfo which is where the actual logic matters (e.g a move operation requires copying to the dest and deleting from the source, a copy operation requires sending the request to the destination bucket region, etc.).

I think I might have underestimated the work by calling this a "slightly larger refactoring". Looking into this, it's actually quite a bit of work to decouple fileinfo from filegenerator. However, long term, I think this is the right direction.

This refactoring removes the necessity of passing arguments through the ``FileGenerator`` class in order for the ``FileInfo`` class to obtain the arguments it requires to perform an operation.

kyleknap · 2014-08-12T16:53:01Z

I agree it was about time to break it up. So here is how I refactored the code. There are two new classes. A FileBase class and an InfoSetter class. The FileBase class is much like a FileInfo but it is much simpler (i.e. only has attributes like src_path, dest_path etc.). These are generated by the FileGenerator class. The FileBase objects are passed to the objects like the IncludeExcludeFilter and the Comparator. Then right before the FileBase hits the S3Handler they are passed to the new class InfoSetter. The InfoSetter creates the FileInfo object by taking the attributes from the FileBase and injecting the attributes relevant to the operation (e.g. endpoint, source_endpoint, and parameters) Then the InfoSetter passes the FileInfo to the S3Handler to perform the operation.

coveralls · 2014-08-12T17:00:44Z

Changes Unknown when pulling c931e54 on kyleknap:source-region into * on aws:develop*.

danielgtaylor · 2014-08-12T17:50:58Z

LGTM, but I'll defer to James for the final review.

jamesls · 2014-08-12T18:24:16Z

tests/integration/customizations/s3/test_plugin.py

@@ -579,6 +594,74 @@ def test_sync_with_delete_option_with_same_prefix(self):
        self.assertEqual('', p.stdout)


+class TestSourceRegion(BaseS3CLICommand):
+    def extra_setup(self):
+        name_comp = []


Is this used anywhere?

Sorry, misread, ignore above comment. Is there any reason you couldn't use the randomly generated names?

According to the issue, this redirect error happens every time when there is a dot in the name or the bucket name is not DNS compatible. So, I decided to make random url names to satisfy the conditions. When I tested it, the error does not occur if the bucket name does not have those properties. The error does happen with the names I generate as shown in the testFailWithoutRegion test

Oh that's right, makes perfect sense. Would you mind adding a comment somewhere about the fact that we're specifically testing non DNS compatible bucket names?

jamesls · 2014-08-12T20:31:20Z

Overall I think this looks good. I think the refactoring to reduce the coupling between the file generator and the actual S3 operations is great. I just have two small things:

Let's switch all the s3 tests in test_plugin.py to use us-west-2 by default.
I like the abstractions you created, but I think the names are pretty vague. For example FileBase makes it seems like it's a base class for file like objects. And InfoSetter is very generic (a class that sets info). Are there maybe more specific terms we can use?

kyleknap · 2014-08-12T20:45:59Z

Sounds good. How about for InfoSetter make it FileInfoBuilder? Then for FileBase how about FileDetails? I really cannot think of any good names for FileBase. Any ideas?

coveralls · 2014-08-12T23:18:26Z

Changes Unknown when pulling b4c932d on kyleknap:source-region into * on aws:develop*.

coveralls · 2014-08-12T23:26:38Z

Coverage increased (+0.09%) when pulling baa8684 on kyleknap:source-region into 2643155 on aws:develop.

jamesls · 2014-08-13T19:07:30Z

tests/integration/customizations/s3/test_plugin.py

@@ -150,7 +172,7 @@ class TestMoveCommand(BaseS3CLICommand):
    def test_mv_local_to_s3(self):
        bucket_name = self.create_bucket()
        full_path = self.files.create_file('foo.txt', 'this is foo.txt')
-        p = aws('s3 mv %s s3://%s/foo.txt' % (full_path,
+        p = _aws('s3 mv %s s3://%s/foo.txt' % (full_path,


These small changes make it hard to review what's actually functionally changed. Another way to accomplish the same thing is to import aws as _aws, and then define def aws in this module that calls to _aws.

jamesls · 2014-08-13T19:16:55Z

looks good. Just a few small changes, but feel free to merge once feedback's been incorporated.

Also moved the ``test_plugin.py`` tests out of ``us-east-1`` and into ``us-west-2``.

coveralls · 2014-08-13T20:38:10Z

Coverage increased (+0.09%) when pulling 8e832a6 on kyleknap:source-region into a6a3c2d on aws:develop.

Added a ``--source-region`` parameter.

Added a --source-region parameter.

dc5c6f2

This parameter ensures the ability to do trans-region syncs, moves, and copies. Tests were expanded as well to better test handling endpoints.

Add InfoSetter class.

c931e54

This refactoring removes the necessity of passing arguments through the ``FileGenerator`` class in order for the ``FileInfo`` class to obtain the arguments it requires to perform an operation.

jamesls reviewed Aug 12, 2014
View reviewed changes

jamesls reviewed Aug 13, 2014
View reviewed changes

Renamed the recently added classes.

8e832a6

Also moved the ``test_plugin.py`` tests out of ``us-east-1`` and into ``us-west-2``.

kyleknap added a commit that referenced this pull request Aug 13, 2014

Merge pull request #872 from kyleknap/source-region

29a797b

Added a ``--source-region`` parameter.

kyleknap merged commit 29a797b into aws:develop Aug 13, 2014

kyleknap deleted the source-region branch August 13, 2014 21:05

kyleknap mentioned this pull request Aug 20, 2014

s3 subcommand has nebulous error message on some buckets #674

Closed

joe4dev mentioned this pull request Nov 30, 2022

bug: s3 mv with --source-region causes fatal error localstack/localstack#6915

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a `--source-region` parameter. #872

Added a `--source-region` parameter. #872

kyleknap commented Aug 11, 2014

kyleknap commented Aug 11, 2014

kyleknap commented Aug 11, 2014

jamesls commented Aug 11, 2014

kyleknap commented Aug 11, 2014

kyleknap commented Aug 11, 2014

jamesls commented Aug 11, 2014

kyleknap commented Aug 12, 2014

coveralls commented Aug 12, 2014

danielgtaylor commented Aug 12, 2014

jamesls Aug 12, 2014

jamesls Aug 12, 2014

kyleknap Aug 12, 2014

jamesls Aug 12, 2014

jamesls commented Aug 12, 2014

kyleknap commented Aug 12, 2014

coveralls commented Aug 12, 2014

coveralls commented Aug 12, 2014

jamesls Aug 13, 2014

jamesls commented Aug 13, 2014

coveralls commented Aug 13, 2014

Added a --source-region parameter. #872

Added a --source-region parameter. #872

Conversation

kyleknap commented Aug 11, 2014

kyleknap commented Aug 11, 2014

kyleknap commented Aug 11, 2014

jamesls commented Aug 11, 2014

kyleknap commented Aug 11, 2014

kyleknap commented Aug 11, 2014

jamesls commented Aug 11, 2014

kyleknap commented Aug 12, 2014

coveralls commented Aug 12, 2014

danielgtaylor commented Aug 12, 2014

jamesls Aug 12, 2014

Choose a reason for hiding this comment

jamesls Aug 12, 2014

Choose a reason for hiding this comment

kyleknap Aug 12, 2014

Choose a reason for hiding this comment

jamesls Aug 12, 2014

Choose a reason for hiding this comment

jamesls commented Aug 12, 2014

kyleknap commented Aug 12, 2014

coveralls commented Aug 12, 2014

coveralls commented Aug 12, 2014

jamesls Aug 13, 2014

Choose a reason for hiding this comment

jamesls commented Aug 13, 2014

coveralls commented Aug 13, 2014

Added a `--source-region` parameter. #872

Added a `--source-region` parameter. #872