Add back support for --endpoint-url option to s3 #469

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@mguillaume
Contributor

mguillaume commented Nov 5, 2013

The --endpoint-url option was re-enabled with commit d6c46df but s3 uses different code to get the endpoint.
This adds back endpoint-url support for the s3 service.

@mguillaume

This comment has been minimized.

Show comment Hide comment
@mguillaume

mguillaume Nov 6, 2013

Contributor

Looks like I have to adapt the unit tests too, I'll update ASAP.

Contributor

mguillaume commented Nov 6, 2013

Looks like I have to adapt the unit tests too, I'll update ASAP.

@jamesls

This comment has been minimized.

Show comment Hide comment
@jamesls

jamesls Nov 6, 2013

Member

Could you also add a unit tests for your change? That way we can ensure we don't regress on this again in the future.

Thanks.

Member

jamesls commented Nov 6, 2013

Could you also add a unit tests for your change? That way we can ensure we don't regress on this again in the future.

Thanks.

@mguillaume

This comment has been minimized.

Show comment Hide comment
@mguillaume

mguillaume Nov 6, 2013

Contributor

I think this one does it.
Note that for S3, without specifying a region, endpoint-url will not work as botocore has to do a lot of "magic" associated with region endpoints and DNS compatible endpoints. I have a (pretty simple) pull request in botocore to allow us to use --region=mock for this: boto/botocore#171

Contributor

mguillaume commented Nov 6, 2013

I think this one does it.
Note that for S3, without specifying a region, endpoint-url will not work as botocore has to do a lot of "magic" associated with region endpoints and DNS compatible endpoints. I have a (pretty simple) pull request in botocore to allow us to use --region=mock for this: boto/botocore#171

@jamesls

This comment has been minimized.

Show comment Hide comment
@jamesls

jamesls Nov 7, 2013

Member

Thanks for adding the test, looks good. Looks like there were other changes to s3 recently so there are a few merge conflicts I'll have to work through. Unless of course you wouldn't mind rebasing against develop :)

Member

jamesls commented Nov 7, 2013

Thanks for adding the test, looks good. Looks like there were other changes to s3 recently so there are a few merge conflicts I'll have to work through. Unless of course you wouldn't mind rebasing against develop :)

@mguillaume

This comment has been minimized.

Show comment Hide comment
@mguillaume

mguillaume Nov 8, 2013

Contributor

This is "rebased" (manually + push -f), but one test does not pass anymore.
I'm not 100% sure but I think the Mock service for s3 doesn't provide everything that is required by the new ListCommand.

Contributor

mguillaume commented Nov 8, 2013

This is "rebased" (manually + push -f), but one test does not pass anymore.
I'm not 100% sure but I think the Mock service for s3 doesn't provide everything that is required by the new ListCommand.

@jamesls jamesls closed this in 3a97a37 Nov 18, 2013

@jamesls

This comment has been minimized.

Show comment Hide comment
@jamesls

jamesls Nov 18, 2013

Member

Sorry for the delay, this has been merged, thanks!

Member

jamesls commented Nov 18, 2013

Sorry for the delay, this has been merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment