search

thumbor-community / aws

Thumbor AWS extensions
MIT License
155 stars 70 forks source link

Use pytest instead of deprecated nose #153

Closed peterrus closed 2 years ago

peterrus commented 2 years ago

With the intention of letting test pass so we can work on https://github.com/thumbor-community/aws/pull/147. In theory nose should work with Python3 but it's a dead project and pytest should be plug and play in this case (tested it locally, seems fine, and the output is a lot nice imho).

@Bladrak If CircleCI is still working and you have merged https://github.com/Bladrak/docker-thumbor-dev/pull/1 we should get passing tests again by merging this into #147

Since #147 is not coming from a fork I have push access to could @amanagr maybe give me access to do so?

Alternatively, if @amanagr is not working on/interested in this project anymore maybe @Bladrak can convert #147 to a branch of this repo to be able to easier manage PR's that work on making tc_aws python3 compatible.

I think this could also just be integrated in to master as it should be compatible with python2. It's just that merging only this branch won't fix CI tests, we need https://github.com/Bladrak/docker-thumbor-dev/pull/1 for that as well.

Bladrak commented 2 years ago

Changed the base to py3, once py3 is stable we can pre-release it

Bladrak commented 2 years ago

Also, waiting for feedback on https://github.com/Bladrak/docker-thumbor-dev/pull/1 and integrating the new image in this MR before merging it

Bladrak commented 2 years ago

@peterrus new image is in, looks like there's some dependencies issues

peterrus commented 2 years ago

@Bladrak could it be that CircleCI is using the config from master instead of this branch? Because it tries to run the python2 docker image (and it should use the new python3 image).

It should use this one: https://github.com/thumbor-community/aws/blob/py3/.circleci/config.yml But it is using this one: https://app.circleci.com/projects/github/thumbor-community/aws/config/?branchName=pull%2F153&pipelineNumber=13

I am not very familiar with CircleCI but from a quick search it looks like it always picks the config from master and not the current branch: https://discuss.circleci.com/t/can-the-circleci-config-be-changed-based-on-the-branch-in-use/23166

That would be problematic because we only want to use the python3 image on python3 branches. We could install python2 and 3 alongside in a shared image and work something out using virtual env's but this would take a bit more work.

Another option would be to drop support for Python2 in tc_aws but you might feel that is a bit premature which I can understand.

Bladrak commented 2 years ago

@peterrus might be because I changed the branch of the MR afterwards and Circle is messing up

Bladrak commented 2 years ago

Honestly I'm not keen on supporting py2 anymore, but we'd need a stable py3 version before 😅

peterrus commented 2 years ago

Never mind @Bladrak :joy: I had a small lapse of reason. Just had to merge your upstream py3 branch into my fork.

It seems I broke one unit test by moving to nose. I will fix that ASAP.

peterrus commented 2 years ago

@Bladrak Voila!

Some funky dependency resolution going in on with regards to boto but nothing blocking! Tests pass now.

Regarding your suggestion edit in the version file: Why do we set it to 4 and not something higher than 6.2.15 ?

Bladrak commented 2 years ago

Sorry, you're right for the version, maybe 7.0b?

peterrus commented 2 years ago

@Bladrak sounds good! Somewhat in sync with Thumbor's versioning

Bladrak commented 2 years ago

@peterrus looks like we have an issue on the deploy script, maybe related to the image, what do you think? https://app.circleci.com/pipelines/github/thumbor-community/aws/20/workflows/7a946629-23c4-45ff-82db-aa4e98bf4154/jobs/283

peterrus commented 2 years ago

Will fix! Strange that this only manifests itself now as I saw some successful runs a few minutes back. https://app.circleci.com/pipelines/github/thumbor-community/aws/18/workflows/28a3b468-fab3-48f8-addf-d94295ee4e3e/jobs/281 for example

Bladrak commented 2 years ago

Yeah I think it's because the one I linked is on a branch on the repo and tries to release it, the other one might be on the PR so not trying to release it.

peterrus commented 2 years ago

@Bladrak ^ should fix it

Bladrak commented 2 years ago

@peterrus we're probably missing ssh as well Edit: my bad, the image wasn't up to date. The issue is different now, let me check

peterrus commented 2 years ago

Yeah ssh should be installed as a dependency of git. Let me know whats up.

Bladrak commented 2 years ago

The deploy key expired on the repo, I re-generated it and it seems to be fine :)

peterrus commented 2 years ago

Awesome! @Bladrak Will this also publish a pip package or is that only done for master ?

Bladrak commented 2 years ago

I thought so but apparently it didn't. I need to check the circle config

peterrus commented 2 years ago

Alright. Let me know if you need any help, happy to do so. Thanks for your quick reactions thusfar!

Bladrak commented 2 years ago

Released on pypi: https://pypi.org/project/tc-aws/7.0b0/ https://pypi.org/project/tc-aws/7.0b0/ Thanks for the contribution!