Skip to content
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

Fix the bug that can not remove the file on aws #124

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

Tideorz
Copy link

@Tideorz Tideorz commented Aug 31, 2018

This fix is used to fix the Issue #123 , which I found that you can't delete the file when using delete api. As the remove() without decorator @return_future, and it will never be executed.
My test environment:
Python 2.7.13

You can easily test by using localstack.

pip install localstack thumbor==6.1.5 botocore==1.5.70 tc-aws==6.2.10
export SERVICES=s3
export DEBUG=1
localstack start

create a 'test' bucket on localstack.

import boto3
s3 = boto3.client('s3', endpoint_url='http://localhost:4572', use_ssl=False)
s3.create_bucket(Bucket='test')

change thumbor.conf using localstack s3.
TC_AWS_ENDPOINT='http://localhost:4572'

Upload and image to 'test' bucket and then delete.

@Bladrak
Copy link

Bladrak commented Aug 31, 2018

Hello and thanks for the contribution! Seems good so far, could you update the test https://github.com/thumbor-community/aws/blob/master/vows/storage_vows.py#L92 accordingly?
Thanks again!

@Tideorz
Copy link
Author

Tideorz commented Sep 1, 2018

hey @Bladrak, I've do some changes to CanRemoveImage, please take a look :)

Copy link

@Bladrak Bladrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside that seems good!

setup.py Outdated
@@ -57,6 +57,7 @@ def readme():
install_requires=[
'python-dateutil',
'thumbor>=6.0.0,<7',
'botocore==1.5.70',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't fix the versions in here, we need to be compatible. What was the error you encountered?

@Tideorz
Copy link
Author

Tideorz commented Sep 3, 2018

hi @Bladrak , I'm tring to fix the broken test cases. After checking broken console log in the former commit. I did some changes in the latest commit. Add botocore==1.5.70 into install_requires. moto==1.3.3 to extras_require.

The reason why I do this, is because some broken tese cases shows as follow:

Raises if invalid config
        ✗ should be an error
            ("Expected topic('Endpoint' object has no attribute 'verify') to be an error of type <type 'exceptions.RuntimeError'>, but it was a <type 'exceptions.AttributeError'>", AttributeError("'Endpoint' object has no attribute 'verify'",), <type 'exceptions.RuntimeError'>, <type 'exceptions.AttributeError'>)
            
Traceback (most recent call last):
              File "/usr/local/lib/python2.7/site-packages/pyvows/runner/abc.py", line 65, in run_vow
    result = vow(ctx_obj, topic)
              File "/root/project/vows/storage_vows.py", line 190, in should_be_an_error
    expect(topic).to_be_an_error_like(RuntimeError)
              File "/usr/local/lib/python2.7/site-packages/preggy/core.py", line 285, in _assert_topic
    return _registered_assertions[method_name](self.topic, *args, **kw)
              File "/usr/local/lib/python2.7/site-packages/preggy/core.py", line 58, in wrapper
    func(*args, **kw)
              File "/usr/local/lib/python2.7/site-packages/preggy/assertions/types/errors.py", line 25, in to_be_an_error_like
    raise err
AssertionError: ("Expected topic('Endpoint' object has no attribute 'verify') to be an error of type <type 'exceptions.RuntimeError'>, but it was a <type 'exceptions.AttributeError'>", AttributeError("'Endpoint' object has no attribute 'verify'",), <type 'exceptions.RuntimeError'>, <type 'exceptions.AttributeError'>)

And i checked the tornado_botocore code.
https://github.com/nanvel/tornado-botocore/blob/master/tornado_botocore/base.py#L105
It will raise Exception('Endpoint' object has no attribute 'verify'.) when try to log debug.
After stepping into code, I found tornado_botocore will finally use botocore's Endpoint class,
https://github.com/boto/botocore/blob/develop/botocore/endpoint.py#L73
And you can see that the Endpoint class doesn't have the verify() method. which makes test broken.
But in botocore==1.5.70, Endpoint class has verify().
https://pypi.org/project/botocore/1.5.70/
So I changed the botocore version to 1.5.70, which is the latest version which supports verify().

And for the moto==1.3.3, I found that in my local environment, after use the latest moto-1.3.5, it will shows

pyvows -c -l tc_aws
Traceback (most recent call last):
  File "/data/home/china/tzhu/local/image-service/environ/bin/pyvows", line 11, in <module>
    sys.exit(main())
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/pyvows/cli.py", line 191, in main
    capture_output=arguments.capture_output
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/pyvows/cli.py", line 143, in run
    Vows.collect(path, pattern)
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/pyvows/core.py", line 166, in collect
    __import__(module_name)
  File "/data/home/china/tzhu/merge/aws/vows/storage_vows.py", line 14, in <module>
    from moto import mock_s3_deprecated as mock_s3
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/moto/__init__.py", line 38, in <module>
    from .secretsmanager import mock_secretsmanager  # flake8: noqa
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/moto/secretsmanager/__init__.py", line 5, in <module>
    secretsmanager_backend = secretsmanager_backends['us-east-1']
KeyError: u'us-east-1'
make: *** [test] Error 1

On Stackoverflow, someone suggested that upgrades you botocore's version to latest. But I can't, so i
try this version moto-1.3.3, which can work with the botocore==1.5.70.

Now all the test cases passed. Not sure whether there is a better solution. If you have good idea, tell me,
Thanks :)

@Bladrak
Copy link

Bladrak commented Sep 3, 2018

Ok, for the first issue, I think we should rather submit a PR on tornado_botocore to update the code to remove the verify call (given it's only for the logger).
For the second, it might be related with your local configuration of S3 (maybe you're using a different region than the default)?

@roxylu
Copy link

roxylu commented Sep 3, 2018

Hi @Bladrak,

I am afraid the following error is not related to region settings:

Traceback (most recent call last):
  File "/data/home/china/tzhu/local/image-service/environ/bin/pyvows", line 11, in <module>
    sys.exit(main())
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/pyvows/cli.py", line 191, in main
    capture_output=arguments.capture_output
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/pyvows/cli.py", line 143, in run
    Vows.collect(path, pattern)
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/pyvows/core.py", line 166, in collect
    __import__(module_name)
  File "/data/home/china/tzhu/merge/aws/vows/storage_vows.py", line 14, in <module>
    from moto import mock_s3_deprecated as mock_s3
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/moto/__init__.py", line 38, in <module>
    from .secretsmanager import mock_secretsmanager  # flake8: noqa
  File "/data/home/china/tzhu/local/image-service/environ/lib/python2.7/site-packages/moto/secretsmanager/__init__.py", line 5, in <module>
    secretsmanager_backend = secretsmanager_backends['us-east-1']
KeyError: u'us-east-1'

See issue here: getmoto/moto#1767

Cheers,
Roxy

@Tideorz
Copy link
Author

Tideorz commented Sep 3, 2018

@Bladrak , I changed the botocore==1.5.70 to extras_requires, now this package installed only influences the CI environment. And I found that, comment the log.debug in tornado_botocore will still be failed in another method(), shows'URLLib3Session' object has no attribute 'get_adapter', seems it's not a simple issue. I'll raise an issue to tornado_botocore, and put the issue link here. But need write simple example for reproducing the bug, which will take some time, not sure when this will be fixed :( . Can you please help to merge this to master, and push to pypi, as my project need use this package recently.

Thanks :D

@Tideorz
Copy link
Author

Tideorz commented Sep 4, 2018

@Bladrak , I've change the setup.py back, Now the code should be safe to be merged. Test cases are broken is because of the compatible issue between latest tornado_botocore and botocore. Not only my branch, the master code also will get broken CI status. I've raised a Issue nanvel/tornado-botocore#18 for this bug, Hope someone will help to fix it. Do you think everything is ok to merge to master ? :p

@Bladrak
Copy link

Bladrak commented Sep 4, 2018

Thanks @Tideorz I'll take a look at the tests. Won't be able to release it to pypi now though, I'll need to fix the tests first as the release is automated :)

@Bladrak
Copy link

Bladrak commented Sep 4, 2018

@Tideorz I've set the versions constraints while we wait for the issue nanvel/tornado-botocore#18 to be fixed (as you said, it seems tornado botocore is not compatible with the latest botocore versions, and moto require the most recent versions of botocore to work, so we'll have to wait before removing those constraints). If you can rebase your PR (and maybe squash it as well) to ensure your tests pass, we shall be able to merge it then :-)

@Tideorz
Copy link
Author

Tideorz commented Sep 5, 2018

hey @Bladrak , I've rebased my PR and squashed the commits. Now test passed. :-)

@Bladrak Bladrak merged commit 7b40255 into thumbor-community:master Sep 5, 2018
@Bladrak
Copy link

Bladrak commented Sep 5, 2018

Thanks for the contribution @Tideorz :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants