Skip to content

Comments

ref(django-3.2): Vendor django picklefield#35727

Merged
AniketDas-Tekky merged 7 commits intomasterfrom
adas/django-32-upgrade/django-picklefield-vendor
Jun 21, 2022
Merged

ref(django-3.2): Vendor django picklefield#35727
AniketDas-Tekky merged 7 commits intomasterfrom
adas/django-32-upgrade/django-picklefield-vendor

Conversation

@AniketDas-Tekky
Copy link
Contributor

@AniketDas-Tekky AniketDas-Tekky commented Jun 16, 2022

Django-picklefield hasn't been updated in 2 years, but we need it. We also need to upgrade to django 3.2 which means we need to update picklefield.

Could use some direction on the direction struture changes

Django-picklefield hasn't been updated in 2 years, but we need it. We also need to upgrade to django 3.2 which means we need to update picklefield.
@evanpurkhiser
Copy link
Member

Where are we using this? How feasible would it be to kill off usage of pickle in favor of JSON?

@AniketDas-Tekky
Copy link
Contributor Author

Where are we using this? How feasible would it be to kill off usage of pickle in favor of JSON?

We use it in authentication and all option models. I think we should remove this, but it's outside the scope of django 3.2 upgrade.

@@ -0,0 +1 @@
DEFAULT_PROTOCOL = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

the package name needs to be an underscore -- that's why the testsuite is failing

@chadwhitacre
Copy link
Member

Can we fork and maintain?

@AniketDas-Tekky
Copy link
Contributor Author

Can we fork and maintain?

Long term, this is the right approach (or removing it entirely). However, for django 3.2 upgrade, we only need to update a couple of lines to make it compatible. So this is a quick fix, until we get some time to set up our own fork.

FWIW, I did fork it already, but there's no CI. https://github.com/getsentry/django-picklefield

@chadwhitacre
Copy link
Member

we only need to update a couple of lines to make it compatible

Can we make the changes in the fork and reference it via a git URL in requirements.txt? Avoids the overhead of releasing but keeps us on the right path w/ the fork.

@chadwhitacre
Copy link
Member

Also not that hard to set up releases via the publish repo. ;)

@asottile-sentry
Copy link
Contributor

we only need to update a couple of lines to make it compatible

Can we make the changes in the fork and reference it via a git URL in requirements.txt? Avoids the overhead of releasing but keeps us on the right path w/ the fork.

at the moment our requirements require pkg==... and forbid git urls (because we can't publish with them)

we could publish a sentry-django-picklefield though

@joshuarli
Copy link
Member

joshuarli commented Jun 17, 2022

I still think vendoring is the easiest and best option here, given that:

  • it's such a small amount of code (easy to vendor)
  • MIT license, no issues there I don't think
  • there's potential to remove it in the future and having it vendored might help (flexibility for transitional changes)

Granted, we should try to also upstream any improvements and compat changes we make.

Copy link
Contributor

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

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

@@ -0,0 +1,10 @@
import django.utils.version
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the version related contents of this file, it's not needed anymore.

@AniketDas-Tekky AniketDas-Tekky merged commit 0ad4aa9 into master Jun 21, 2022
@AniketDas-Tekky AniketDas-Tekky deleted the adas/django-32-upgrade/django-picklefield-vendor branch June 21, 2022 17:10
AniketDas-Tekky pushed a commit that referenced this pull request Jun 23, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants