django-commons / django-tasks-scheduler

Schedule async tasks using redis protocol. Redis/ValKey/Dragonfly or any broker using the redis protocol can be used.
https://django-tasks-scheduler.readthedocs.io/
MIT License
58 stars 11 forks source link

Django warning about "makemigrations" #119

Closed gabriels1234 closed 6 months ago

gabriels1234 commented 6 months ago

Describe the bug my_app-web | Your models in app(s): 'scheduler' have changes that are not yet reflected in a migration, and so won't be applied. my_app-web | Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

Expected behavior I did run makemigrations, but did not apply it, to prevent future messing up with official migrations. likely the migrations should come from the package, and perhaps this would prevent issues.

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information): django-tasks-scheduler==1.3.2

Thanks so much!!!!

cunla commented 6 months ago

What is the latest migration you have for scheduler?

cunla commented 6 months ago

and can you send me the result of makemigrations?

gabriels1234 commented 6 months ago

last migration is 17 this is 18 (makemigrations):

Generated by Django 5.0.3 on 2024-05-10 12:45

from django.db import migrations, models

class Migration(migrations.Migration):

dependencies = [
    ('scheduler', '0017_remove_crontask_repeat_crontask_failed_runs_and_more'),
]

operations = [
    migrations.AlterField(
        model_name='crontask',
        name='queue',
        field=models.CharField(choices=[('default', 'default')], help_text='Queue name', max_length=255, verbose_name='queue'),
    ),
    migrations.AlterField(
        model_name='repeatabletask',
        name='queue',
        field=models.CharField(choices=[('default', 'default')], help_text='Queue name', max_length=255, verbose_name='queue'),
    ),
    migrations.AlterField(
        model_name='scheduledtask',
        name='queue',
        field=models.CharField(choices=[('default', 'default')], help_text='Queue name', max_length=255, verbose_name='queue'),
    ),
]
cunla commented 6 months ago

Oh, I see.

This is happening because you have different settings.QUEUES so the new migration "changes" the BaseTask.queue field so you can see all the options from settings.QUEUES in the dropdown list when creating a task through the admin view.

i.e., it is not a bug - it is a feature :)

I should document it.

gabriels1234 commented 6 months ago

So how do you make so it does not conflict with further scheduler app migrations you might add in the future. Should I Apply this one migration?

Get Outlook for iOShttps://aka.ms/o0ukef


From: Daniel M @.> Sent: Friday, May 10, 2024 8:51:54 AM To: dsoftwareinc/django-tasks-scheduler @.> Cc: Salem, Gabriel @.>; Author @.> Subject: Re: [dsoftwareinc/django-tasks-scheduler] Django warning about "makemigrations" (Issue #119)

Oh, I see.

This is happening because you have different settings.QUEUES so the new migration "changes" the BaseTask.queue field so you can see all the options from settings.QUEUES in the dropdown list when creating a task through the admin view.

i.e., it is not a bug - it is a feature :)

I should document it.

— Reply to this email directly, view it on GitHubhttps://github.com/dsoftwareinc/django-tasks-scheduler/issues/119#issuecomment-2104556157, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AW464QPLC5PQGGWQOEWU6ILZBS7GVAVCNFSM6AAAAABHPCIECSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBUGU2TMMJVG4. You are receiving this because you authored the thread.Message ID: @.***>

cunla commented 6 months ago

That's a good point! In django 5 they added support for choices to be a callable which would resolve it - but then it would not support django 4... I will think about it.

gabriels1234 commented 6 months ago

I saw the change for 1.3.3.

-    QUEUES = [(key, key) for key in settings.QUEUES.keys()]
+    QUEUES = [("default", "default"), ("low", "low"), ("high", "high")]

It would break things in many of my projects.

How about this?

class BaseTask(models.Model):
    # Other fields remain unchanged

    @property
    def QUEUES(self):
        """Generate queue choices dynamically from settings to prevent migrations."""
        return [(key, key) for key in settings.QUEUES.keys()]

    queue = models.CharField(
        _('queue'), max_length=255, choices=property(lambda self: self.QUEUES),
        help_text=_('Queue name'), )

This is supposed to work on v4 as well (haven't tested).

Do you want me to submit a PR and do the proper testing? main goal would be to make it the best possible without breaking v4 compatibility.


EDIT:

this is another option if the above was incorrect:

import django
class BaseTask(models.Model):
    #...

    def get_queue_choices(self):
        """Return queue choices directly from settings, avoiding migrations for changes."""
        return [(key, key) for key in settings.QUEUES.keys()]

    if django.VERSION >= (5, 0):
        # For Django 5.0 and above, use a callable directly for dynamic choices.
        queue = models.CharField(
            _('queue'), max_length=255, choices=get_queue_choices,
            help_text=_('Queue name'), )
    else:
        # For versions before Django 5.0, assign choices at model load time.
        queue_choices = get_queue_choices()
        queue = models.CharField(
            _('queue'), max_length=255, choices=queue_choices,
            help_text=_('Queue name'), )
cunla commented 6 months ago

It should not break things. Notice there is another change in ready() (here)

which updates the value once the project loads.