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

Replace call to self._clean_name with clean_name #252

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Replace call to self._clean_name with clean_name #252

merged 2 commits into from
Mar 31, 2023

Conversation

agsimmons
Copy link
Contributor

Fixes #251

This utility function was moved in April 2017, so it should be supported on all recent versions of django-storages, but it would break support for versions older than that. Should a minimum django-storages version be specified for django-s3file?

clean_name utility function was first made available in version 1.6
@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration /language:javascript. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (df1b4df) 98.02% compared to head (c2036fe) 98.03%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #252   +/-   ##
=======================================
  Coverage   98.02%   98.03%           
=======================================
  Files           8        8           
  Lines         203      204    +1     
=======================================
+ Hits          199      200    +1     
  Misses          4        4           
Impacted Files Coverage Δ
s3file/middleware.py 93.61% <ø> (ø)
s3file/storages_optimized.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration /language:python. As part of the setup process, we have scanned this repository and found 1 existing alert. Please check the repository Security tab to see all alerts.

Copy link
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @agsimmons,

Thanks for the patch. I believe we can safely limit the django-storages dependency. After all, your package manager will inform you about compatibility, and we're not forcing users to a new major version.

I am curious, that we didn't notice this before. Would you be so kind and add a test that covers this better?

Thanks again!

Cheers,
Joe

@agsimmons
Copy link
Contributor Author

storages.utils.clean_name was first made available in django-storages 1.6 but storages.backends.s3boto3.S3Boto3Storage._clean_name was just removed in the most recent django-storages 1.13.2 which only came out 3 months ago.

The dependabot PRs from the past 2 months have all had failing tests because of this. Here's one example: https://github.com/codingjoe/django-s3file/actions/runs/3839867555/jobs/6538228693

I increased the minimum django-storages version to 1.6 in this PR, but I'm wondering if maybe more minimum dependencies should be set. Django 3.2 is the oldest version receiving security updates, it's the earliest version being tested against this project, and support for it was added in django-storages 1.12. Maybe these potential changes are better suited for a separate PR.

Copy link
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agsimmons thanks for the swift response. I didn't notice tests failing. I am probably maintaining too many things. Therefore, I really appreciate people like you, making excellent contributions. 🥇

@codingjoe codingjoe merged commit 660d079 into codingjoe:main Mar 31, 2023
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.

S3OptimizedUploadStorage: self._clean_name does not exist
2 participants