Refactor to use aiobotocore, python3 and tornado 7

[amanagr]

This is https://github.com/thumbor-community/aws/pull/142 without the last commit which tries to use aiobotocore's context manager approach and this PR uses aiobotocore v0.12.

Testing on s3 worked fine for me.

[bagipriyank]

the build is failing because the thumbor-dev docker image used for building and testing is using python2 instead of python3. the docker image is built using https://github.com/Bladrak/docker-thumbor-dev/blob/master/Dockerfile. can we move over the project under thumbor-community as well? i can help set it up with python3 in that case so we can move this pr forward.

[Bladrak]

Hi @bagipriyank sorry for the delay, I I'll take a look at this docker image issue.

[Bladrak]

You should be able to switch to https://hub.docker.com/r/bladrak/thumbor-dev-py3 (which is essentially the same image as before, but for python 3 :) )

[bagipriyank]

https://github.com/thumbor-community/aws/pull/149. maybe @amanagr can just pull changes from that pr into this one?

[amanagr]

Done.

[amanagr]

@bagipriyank feel free to work on top of this PR.

[Bladrak]

BC Breaks should be documented (for instance the removal of the presigning loader)

[bagipriyank]

stupid question, how do i push changes to this pull request?

[amanagr]

I don't think you can unless you are maintainer of this repository. You can open a parallel pull request I suppose.

[Bladrak]

It's possible if you give @bagipriyank write access to your forked repo @amanagr :)

[bagipriyank]

another option is to close this pr, push @amanagr's changes to a branch py3 in this repo and open a pr between py3 branch and master. then i can keep pushing against that branch and once everything looks ok we can merge the new pr.

[amanagr]

@bagipriyank sounds good to me.

[amanagr]

@bagipriyank any update on this?

[timabbott]

Any news on reviewing and integrating this?

[RaJiska]

Needed here too. Any news ?

[jimas14]

Also looking for this. Is it possible to get a release here before thumbor 7.0 is stable?

[jimas14]

FYI -- when running this code I see this error for each request

2021-05-04 19:50:31 asyncio:ERROR Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x7f319b6bde48>

[jimas14]

@amanagr Please check the above PR https://github.com/amanagr/aws/pull/1 when you get a chance and let me know what you think!

[dominik-bln]

Would it be possible to publish a prerelease with these changes? Initial testing looks good, but before I promote this to our staging environment it would be nice to be able to at least refer to a tag or branch in this repository instead of the fork.

[Bladrak]

Not sure I can since the branch is not on this repo.

[peterrus]

@Bladrak Do you not have the correct permissions to merge this PR or? If you're maintainer you should be able to merge a PR afaik.

[Bladrak]

@peterrus the MR is still a WIP, as it's not passing the tests and there are some feedbacks to take into account. So I can't merge it until it's stable (or I risk having a master branch unstable). We would have a pre-release if the same MR is on another branch than master, then I could merge in an unstable branch. This would require someone to submit this PR to another branch. I'm not sur @amanagr is still working on this so if someone is up to the task, it would be great the create a new MR from this one.

[peterrus]

@Bladrak of course, I understand, apologies. I did not see the feedback in #142. I gave this branch a try in a test environment and saw no regression in functionality except the issues regarding unclosed connections that @jimas14 has fixed in https://github.com/amanagr/aws/pull/1. Running from that branch solved that as well.

Just some feedback (:

Furthermore it seems this branch has locked aiobotocore==0.12.0 which in turn means you will install a vulnerable version of urllib3==1.24.3. I force-installed urllib3==1.26.8 and this did not seem to give any issues. Judging from urllib3's change log there are no breaking changes.

[Bladrak]

Thanks for the feedback @peterrus :-) Would you be able to submit a new PR with the fixes you've mentioned? As soon as this is passing and the feedbacks are documented, I'm willing to merge :)

[peterrus]

Hi @Bladrak I did not do any fixes except for installing some packages through pip. I am just using aiobotocore==0.12.0 which depends on a vulnerable version of urllib3 so I force installed a newer version as described above.

I am not sure if I feel confident enough in fixing the issues from the feedback of #142 as I have never worked on either Thumbor's or this plugins sources before unfortunately.

[peterrus]

If we get the two referenced PR's :arrow_up: merged we should have working unit tests both locally and on CircleCI again.

[Bladrak]

I switched the base branch of this from master to py3 so we can pre-release it, merging it as is, considering py3 is an unstable branch

[mello7tre]

Seems that not all relevant commits have been merged: if i use https://github.com/thumbor-community/aws/tree/py3 i get:

2022-02-21 11:07:28 asyncio:ERROR Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x7f9eec0affd0>
2022-02-21 11:07:28 asyncio:ERROR Unclosed connector
connections: ['[(<aiohttp.client_proto.ResponseHandler object at 0x7f9eec2cea00>, 4350387.294399503)]']
connector: <aiohttp.connector.TCPConnector object at 0x7f9eec0affa0>

error message disappear if i use direclty: https://github.com/amanagr/aws/tree/thumbor-7

[peterrus]

You are right @mello7tre. Seems https://github.com/amanagr/aws/pull/1 was not merged into py3 cc @Bladrak

[mello7tre]

Thanks. Just for info, iam doing some test with locust and performance of thumbor 7.0.6 with tc_aws from https://github.com/amanagr/aws/tree/thumbor-7 is much better than latest thumbor 6 + tc_aws.

[Bladrak]

Yes indeed, if someone can open a PR on this repo for https://github.com/amanagr/aws/pull/1 into the py3 branch I'll gladly review it!

[peterrus]

Done @Bladrak

Changes were already reviewed and semi-approved I think, but I'll leave that up to you and @jimas14.

[Bladrak]

Thanks, merged!