-
Notifications
You must be signed in to change notification settings - Fork 314
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
Make S3 support optional #1252
Make S3 support optional #1252
Conversation
@@ -70,10 +70,22 @@ def finish(self): | |||
self.p.finish() | |||
|
|||
|
|||
def _fake_import_boto3(): | |||
# This function only exists to be mocked in tests to raise an ImportError, in | |||
# order to simulate the absence of boto3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite ugly, but the alternative is to use del sys.modules["boto3"]
, which is an ugly and global hack, and likely to break things. While we use this hack succesfully in urllib3 to test support for Python versions without ssl, this:
- has caused huge headaches in a project that adds async support to urllib3 and
- would prevent running tests in parallel.
So I would suggest avoiding touching sys.modules
.
@elasticmachine run tests please |
Thanks for running the tests! I'll fix the lint |
@DJRickyB Can you please run the tests again? I guess @elasticmachine won't listen if I try to ask myself |
@elasticmachine run tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pquentin thanks a ton! I have one remaining requirement before approval: please preserve the behavior where make install
ends up installing boto3+dependencies. for our purposes I think it's ok if [s3]
resolution occurs in either make install-user
or a new make
target altogether
@DJRickyB Thank you for the review! Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you once again for your contribution
Thank you for the quick reviews! I'll work on making GCS support optional next. |
@pquentin thank you, but we are looking at keeping GCS support first-class for the time being |
Closes #1095.
With this pull request, boto3 is no longer installed by default but should be installed using
python -m pip install esrally[s3]
instead.When a S3
base-url
is provided, here's what users see:So, we let the
ImportError
propagate, but also explain how to get S3 support.To test this manually in an existing virtual environment, uninstalling boto3 manually is not enough, as
pkg_resource.require("esrally")
will complain that it's missing. Recreating the virtual environment from scratch and removing any .egg-info files should fix this.For what it's worth, I haven't managed to get the integration tests to work locally, but they don't seem to use S3, so they probably still pass.
(This is only about S3 as suggested by #1095, but I'll be happy to do the same thing for GCS if this PR gets merged.)