Add Django 3 support - Githubissues

bufke commented 4 years ago

@tiagocordeiro @klis87 is any help needed reviewing this? The agency I work with is looking into adopting this package to replace one we already forked. https://gitlab.com/thelabnyc/wagtailcloudinary

MaciejWiatr commented 4 years ago

I would really love to see this PR merged, I can also help to review it!

klis87 commented 4 years ago

@MaciejWiatr @bufke I believe PR was not finished, some tests not passing etc. Sadly I cannot spend any time at the moment on it, but I didn't abandon it. If PR is reviewed, I will happily merge it and publish new version. Thx for you help!

bufke commented 4 years ago

Looking at it

Adds Travis CI
Removes six/Python 2 support (I agree with this)
Updates syntax for Django 3.0
Tests appear to pass on Travis CI (from months ago). Example
Two tests fail for me when running locally. I ran them in a python:3.8-slim docker image and had to install libmagic-dev. Then I added some cloudinary api key. Then the tests ran.

py38-dj30 run-test: commands[0] | coverage run -a --source=cloudinary_storage,tests manage.py test
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
..............................................................F.F................
======================================================================
FAIL: test_file_exists_with_the_same_name_as_before_save (tests.tests.test_storage.StaticCloudinaryStorageTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/code/tests/tests/test_storage.py", line 172, in test_file_exists_with_the_same_name_as_before_save
    self.assertTrue(self.storage.exists(self.name))
AssertionError: False is not true

======================================================================
FAIL: test_file_wont_be_uploaded_with_the_same_content (tests.tests.test_storage.StaticCloudinaryStorageTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/unittest/mock.py", line 1325, in patched
    return func(*newargs, **newkeywargs)
  File "/code/tests/tests/test_storage.py", line 177, in test_file_wont_be_uploaded_with_the_same_content
    self.assertFalse(save_mock.called)
AssertionError: True is not false

----------------------------------------------------------------------
Ran 81 tests in 22.527s

FAILED (failures=2)

So is the main mystery figuring out why those two tests no longer pass? Anything else I'm missing?

On the positive side, I manually tested this branch with a wagtail project using both Django 2.2 and 3.0 and it all seemed working.

bufke commented 4 years ago

I wasn't able to get tests to pass without this PR either. Probably either cloudinary API changes or lack of pinned requirements.

I dug into test_file_exists_with_the_same_name_as_before_save

It produces urls like https://res.cloudinary.com/cloud-name/raw/upload/v1/static/2a560a12-5578-482a-a09a-73ded7550a23

But those are not valid urls. Removing the v1 fixes the link. Making it v2 or any other v* string makes it work.

Kind of feels like the bug is in Cloudinary's pycloudinary or it's API itself. This problem seems specific to the "raw" resource type.

I cannot reproduce this from the Cloudinary UI

I upload a txt file (thus raw resource_type).
Get the public URL
Change the "v" to "v1"

Works fine...

Done for today, but I guess next steps would be tracing how this package uploads the raw file.

klis87 commented 4 years ago

@bufke interesting. I am sure that all tests passed on master when I recently released.

Indeed dependencies aren't pinned, looking at requirements.txt and setup.py... Pity, maybe this is a breaking change of pycloudinary? After finding this, we also should pin deps so this won't repeat in the future. Good find, if master breaks then this must be it!

bufke commented 4 years ago

To ease in testing, I uploaded django-cloudinary-storage-thelab that includes this PR. I'm fairly confident it works for Image files and likely may not for raw. @klis87 I've been a fan of using Poetry to version pinning. Of course there are a few solutions to this. If you are interested, I'd be happy to submit a convert to Poetry PR after we figure out the existing issues.

I don't intend to maintain the -thelab package as a fork. To anyone reading - please use it for testing or temporary workaround purposes only. It is not likely to get updated and may get updated with little testing and no notice.

klis87 commented 4 years ago

@bufke It was a while since I worked with python, I generally use js now, I use npm and yarn as package managers and really I hate requirements.txt or setup.py in comparison. I checked poetry and actually I noticed I starred it some time ago. If this has a lock file to pin supdependencies, not only direct dependencies, I would certainly give it a go. Thx for suggesting that!

bufke commented 4 years ago

@klis87 is there anything else here I can do to help? Would you want a PR submitted for Poetry? I'm 80% confident this PR does not break raw and it's an unrelated problem. The uncertainy is that I never used Cloudinary with raw data before so it's hard to tell what used to work vs me not understanding something. Even now I'm planning to use this project for images and not other media.

gathuaalex commented 4 years ago

I'll be glad if this PR is merged.. I'm ecountering problems with django 3.0

klis87 commented 4 years ago

@bufke I would really like to make sure raw files work because then this package can be used to host css and js files too, which was one of my goals.

For now, I merged this and published as 0.3.0 version. I hope it will work, It has been a while since I published any Python package, many things has changed :+1:

Regarding Poetry, I would really appreciate it! Maybe pinning pycloudinary would fix raw file tests too

gathuaalex commented 4 years ago

I bumped back to django 2.2.8 and am serving all my staticfiles from cloudinary.

Alex Gathua Biomedical Engineer ( to graduate)

On Mon, Aug 24, 2020, 1:22 AM Konrad Lisiczyński notifications@github.com wrote:

@bufke https://github.com/bufke I would really like to make sure raw files work because then this package can be used to host css and js files too, which was one of my goals.

For now, I merged this and published as 0.3.0 version. I hope it will work, It has been a while since I published any Python package, many things has changed 👍

Regarding Poetry, I would really appreciate it! Maybe pinning pycloudinary would fix raw file tests too

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/klis87/django-cloudinary-storage/pull/22#issuecomment-678832277, or unsubscribe https://github.com/notifications/unsubscribe-auth/API57I43FUAPHXDJTSSEO63SCGJCTANCNFSM4K7H3OYA .

klis87 commented 4 years ago

@gathuaalex so after this PR it doesnt work with Django 3?

gathuaalex commented 4 years ago

Okay, I'll give it a try when am doing another version of my web app ... I'll give you feedback very soon.Sent from my Huawei phone-------- Original message --------From: Konrad Lisiczyński notifications@github.comDate: Mon, Aug 24, 2020, 11:22 PMTo: klis87/django-cloudinary-storage django-cloudinary-storage@noreply.github.comCc: Alex Gathua alexgathua3@gmail.com, Mention mention@noreply.github.comSubject: Re: [klis87/django-cloudinary-storage] Add Django 3 support (#22) @gathuaalex so after this PR it doesnt work with Django 3?

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

klis87 / django-cloudinary-storage

Add Django 3 support #22