New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying s3 host from boto config file. #3738

Merged
merged 1 commit into from Jun 14, 2017

Conversation

Projects
None yet
2 participants
@houglum
Copy link
Contributor

houglum commented Jun 13, 2017

This allows users to specify a value for s3:host in their Boto config file, rather than needing to pass the host directly to the constructor for S3Connection. There are some situations (e.g. when using gsutil) where users don't have direct control over how the constructor is called; this allows passing the host argument in such scenarios.

(This also fixes an s3 integration test flake.)

@houglum

This comment has been minimized.

Copy link
Contributor

houglum commented Jun 13, 2017

@mfschwartz - could you give this a review?

@houglum

This comment has been minimized.

Copy link
Contributor

houglum commented Jun 13, 2017

no_host_provided = True
host = self.DefaultHost
host = boto.config.get('s3', 'host')
if host is None:

This comment has been minimized.

@mfschwartz

mfschwartz Jun 14, 2017

Member

It would be more Pythonic to do:
if not host:

This comment has been minimized.

@houglum

houglum Jun 14, 2017

Contributor

I'm explicitly checking for NoneType here (which is the default from get() if 'host' isn't found in the boto config).

from our style guide: "Beware of writing if x: when you really mean if x is not None:-e.g., when testing whether a variable or argument that defaults to None was set to some other value. The other value might be a value that's false in a boolean context!"

self.assertIn(
check.cache_control,
('public,%20max-age=500', 'public, max-age=500')
)

This comment has been minimized.

@mfschwartz

mfschwartz Jun 14, 2017

Member

This change looks unrelated to the CL description?

This comment has been minimized.

@houglum

houglum Jun 14, 2017

Contributor

Yep - didn't mention it in the commit message, but I put a little note about it at the bottom of the pull request. It's just been an annoying test "failure" to have to look over every time I run the s3 integration tests.

@mfschwartz mfschwartz merged commit dcfc751 into boto:develop Jun 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment