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

Use pytest instead of deprecated nose #153

Merged
merged 5 commits into from
Feb 9, 2022
Merged

Use pytest instead of deprecated nose #153

merged 5 commits into from
Feb 9, 2022

Conversation

peterrus
Copy link

@peterrus peterrus commented Feb 5, 2022

With the intention of letting test pass so we can work on #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 Bladrak/docker-thumbor-dev#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 Bladrak/docker-thumbor-dev#1 for that as well.

@Bladrak Bladrak changed the base branch from master to py3 February 9, 2022 08:38
@Bladrak
Copy link

Bladrak commented Feb 9, 2022

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

@Bladrak
Copy link

Bladrak commented Feb 9, 2022

Also, waiting for feedback on Bladrak/docker-thumbor-dev#1 and integrating the new image in this MR before merging it

@Bladrak
Copy link

Bladrak commented Feb 9, 2022

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

@peterrus
Copy link
Author

peterrus commented Feb 9, 2022

@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
Copy link

Bladrak commented Feb 9, 2022

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

@Bladrak
Copy link

Bladrak commented Feb 9, 2022

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

@peterrus
Copy link
Author

peterrus commented Feb 9, 2022

Never mind @Bladrak 😂 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.

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.

Unless for the version it looks good!

version.txt Outdated Show resolved Hide resolved
@peterrus
Copy link
Author

peterrus commented Feb 9, 2022

@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 ?

@peterrus peterrus closed this Feb 9, 2022
@peterrus peterrus reopened this Feb 9, 2022
@Bladrak
Copy link

Bladrak commented Feb 9, 2022

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

@peterrus
Copy link
Author

peterrus commented Feb 9, 2022

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

version.txt Outdated Show resolved Hide resolved
@Bladrak Bladrak merged commit 193731d into thumbor-community:py3 Feb 9, 2022
@Bladrak
Copy link

Bladrak commented Feb 9, 2022

@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 peterrus deleted the fix_circleci_python3 branch February 9, 2022 16:36
@peterrus
Copy link
Author

peterrus commented Feb 9, 2022

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
Copy link

Bladrak commented Feb 9, 2022

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
Copy link
Author

peterrus commented Feb 9, 2022

@Bladrak ^ should fix it

@Bladrak
Copy link

Bladrak commented Feb 9, 2022

@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
Copy link
Author

peterrus commented Feb 9, 2022

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

@Bladrak
Copy link

Bladrak commented Feb 9, 2022

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

@peterrus
Copy link
Author

peterrus commented Feb 9, 2022

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

@Bladrak
Copy link

Bladrak commented Feb 9, 2022

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

@peterrus
Copy link
Author

peterrus commented Feb 9, 2022

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

@Bladrak
Copy link

Bladrak commented Feb 9, 2022

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

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.

None yet

2 participants