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.

Sign up for GitHub

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

Jump to bottom

Draft: Switch to Poetry #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Draft: Switch to Poetry #25

wants to merge 1 commit into from

Conversation

bufke
Copy link

@bufke bufke commented Sep 2, 2020 •
edited
Loading

Fixes #24

What does this do?

  • Adds Poetry and pyproject.toml to replace requirements.txt and setup.py
    • I believe all attributes have been ported - notice how it's quite a bit different than setup.py
    • Poetry adds a lock file that works in a similar manner as npm. You can update with poetry update or poetry add package@">=version"
  • Drop Python 3.5
  • Be more explicit about Django 2.2 being the lowest supported version
  • Add Django 3.1 to tests
  • Tools such as codecov are explicitly in dev-dependencies. My opinion is that if CI needs it, then it's a dev dependency.

Things to watch out for

  • I'm not familiar with Travis CI - I hope what I did works but am not sure
  • I have never seen tests pass locally and continue to get 2 errors

Things to consider

  • Poetry can also submit to pypi and I recommend using it for such. In the past I have set up Gitlab CI to publish to pypi using an API token based on tags. That let's me tag a release and publish very easily (and makes it easier to delegate releases to others).
  • Feel free to tweak whatever. This is my suggestion on how to do it.
  • Personally, I love running projects with Docker to avoid having to think about system dependencies. I'd be happy to add a Dockerfile and docker-compose.yml file if you wanted it.

@bufke
Switch to Poetry
92b2099 
Drop Python 3.5
Add Django 3.1 to tests
@bufke
Copy link
Author

bufke commented Sep 2, 2020

I was hoping CI would run when I submit this. But it has not. :(

@klis87
Copy link
Owner

klis87 commented Sep 2, 2020 •
edited
Loading

@bufke Thx! I will review it as soon as I can! Regarding travis I believe due to security reason only merged PRs are run with Travis (it could be configurable though) The reason is that any attacker could for instance make a PR to print env variables with password, I guess this is the main reason :)

With this PR I will also try to understand why tests for raw files dont work. Potentially we should update pycloudinary, pin it and update the app code accordingly. Or... to pin it to a max version for which it does work :)

@klis87
Copy link
Owner

klis87 commented Sep 8, 2020

@bufke Just to inform your about my progress. I studied your PR and read docs for Poetry, indeed this is awesome tool! Thx for suggesting it and for this PR once again! In the following days in some spare time I will play with Poetry more, making sure it will work with Travis and so on. Then, after merging, I will try to fix raw files issue.

@klis87
Copy link
Owner

klis87 commented Sep 9, 2020

@bufke I run test locally and I tried old version of pycloudinary too. Unfortunately I still got 2 tests failed. I am pretty sure there are some issues with collectstaticfiles command, probably due to change from this ticket https://code.djangoproject.com/ticket/28055

So unfortunately fixing those 2 tests will require some more work, I will try to check whats wrong in the near future.

@klis87
Copy link
Owner

klis87 commented Sep 9, 2020

actually this must be a different reason... sth really with StaticCloudinaryStorage...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Switch to Poetry with better dependency pinning
2 participants
@bufke @klis87