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

Make app AutoField configurable on first deployment #465

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

browniebroke
Copy link
Contributor

@browniebroke browniebroke commented Mar 5, 2025

While not fully addressing the problem reported in #307, it switches new installations to use a BigAutoField for primary key. Existing installations won't see any changes unless they replay the migrations and to recreate the result table.

@browniebroke browniebroke force-pushed the fix/results-auto-field-setting branch from 1d87f48 to 0d75c01 Compare March 5, 2025 00:07
@browniebroke browniebroke force-pushed the fix/results-auto-field-setting branch from 0d75c01 to 94380cb Compare March 5, 2025 00:08
@auvipy auvipy self-requested a review March 5, 2025 04:54
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

we should also think about what if existing projects want to migrate to this

@browniebroke browniebroke marked this pull request as ready for review March 5, 2025 08:04
@browniebroke
Copy link
Contributor Author

we should also think about what if existing projects want to migrate to this

Thta's true, but that is more complicated and I don't think I want to tackle migrating existing installations in this pull request.

Such migration might not even be feasible in a database agnostic way, as writing a safe migration might need to use database specific features. The project is tested against Postgres only, but it doesn't seem to use any PG only features? Is it supposed to work on other databases backend supported by Django?

@@ -0,0 +1,19 @@
Configuration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another setting DJANGO_CELERY_RESULTS_TASK_ID_MAX_LENGTH which should probably be added here but I'd rather do this separately if you don't mind.

@auvipy
Copy link
Member

auvipy commented Mar 12, 2025

what is your take on this PR? although it is not 100% design wise #314

@browniebroke
Copy link
Contributor Author

what is your take on this PR? although it is not 100% design wise #314

That could also be a solution to the problem described in #307, although I would argue that's solving an othogonal problem. It's a bit heavy handed for thoses who only want to customise the primary key, and new users usually won't know about the primary key overflow until it's too late.

I'm seeing a fundamental missing piece betwen both PRs: formalising django-celery-results app settings better. What do you think of the implementation in this PR? I took this pattern from this post, which is making a great case for it.

If you have another pattern you prefer, I'm happy to use another example as a blueprint. I can contribute this piece separately first.

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.

2 participants