[Django] #34436: `makemigrations --check` fails with error code 1 if system checks identify warnings
Django
warnings
-----------------------------------------+------------------------
Reporter: James Addison | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 4.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
In working through the upgrade steps from a Django 3.2 based system to
Django 4.2, I encountered behaviour that breaks our github actions
continuous integration (it is intended to fail if there are missing
migrations needed).
In this case, there are no missing migration files identified, but the
`./venv/bin/python manage.py makemigrations --check` command errors out
with `1`, presumably due to CICharField W905 and CITextField W907
warnings:
{{{
# ./venv/bin/python manage.py makemigrations --check
System check identified some issues:
WARNINGS:
accounts.ComplianceRegion.name: (fields.W905)
django.contrib.postgres.fields.CICharField is deprecated. Support for it
(except in historical migrations) will be removed in Django 5.1.
HINT: Use CharField(db_collation="…") with a case-insensitive non-
deterministic collation instead.
accounts.ComplianceRegion.sub_region: (fields.W905)
django.contrib.postgres.fields.CICharField is deprecated. Support for it
(except in historical migrations) will be removed in Django 5.1.
HINT: Use CharField(db_collation="…") with a case-insensitive non-
deterministic collation instead.
information.Degree.degree: (fields.W907)
django.contrib.postgres.fields.CITextField is deprecated. Support for it
(except in historical migrations) will be removed in Django 5.1.
HINT: Use TextField(db_collation="…") with a case-insensitive non-
deterministic collation instead.
System check identified 3 issues (5 silenced).
# echo $?
1
# /venv/bin/python manage.py check
System check identified some issues:
WARNINGS:
accounts.ComplianceRegion.name: (fields.W905)
django.contrib.postgres.fields.CICharField is deprecated. Support for it
(except in historical migrations) will be removed in Django 5.1.
HINT: Use CharField(db_collation="…") with a case-insensitive non-
deterministic collation instead.
accounts.ComplianceRegion.sub_region: (fields.W905)
django.contrib.postgres.fields.CICharField is deprecated. Support for it
(except in historical migrations) will be removed in Django 5.1.
HINT: Use CharField(db_collation="…") with a case-insensitive non-
deterministic collation instead.
information.Degree.degree: (fields.W907)
django.contrib.postgres.fields.CITextField is deprecated. Support for it
(except in historical migrations) will be removed in Django 5.1.
HINT: Use TextField(db_collation="…") with a case-insensitive non-
deterministic collation instead.
System check identified 3 issues (5 silenced).
# echo $?
0
}}}
Note that both `manage.py makemigrations --check` and `manage.py check`
are provided above for clarity.
These are deprecation warnings (to be removed in Django 5.1). I think this
management command should not be erroring out in relation to these at this
point in time.
I personally have time to address these CITextField and CICharField
warnings properly by moving to the preferred `db_collation=...` collation
approach, but wanted to raise this as a bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/34436>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
warnings
Reporter: James Addison | Owner: nobody
Component: Migrations | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => invalid
Comment:
Replying to [ticket:34436 James Addison]:
> Note that both `manage.py makemigrations --check` and `manage.py check`
are provided above for clarity.
>
> These are deprecation warnings (to be removed in Django 5.1). I think
this management command should not be erroring out in relation to these at
this point in time.
`check` cannot succeed when warnings are raised, so a non-zero return code
is expected. You can always use the
[https://docs.djangoproject.com/en/4.1/ref/settings/#std-setting-
SILENCED_SYSTEM_CHECKS SILENCED_SYSTEM_CHECKS] setting to ignore them.
--
Ticket URL: <https://code.djangoproject.com/ticket/34436#comment:1>
Django
warnings
-------------------------------+--------------------------------------
Reporter: James Addison | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by James Addison):
I think this should be reopened - I suspect I didn't convey the meaning
correctly?
- `manage.py check` is ''correctly not failing'' (see the 0 exit code in
my example)
- nothing is currently (until 5.1 is released) in a broken check state
- `manage.py makemigrations --check` is ''incorrectly failing'' (see 1
exit code in my example above)
- the 'check' for missing migrations yielded no missing migrations, so
should not return an error
- the fact that those warning are raised have nothing to do with
migrations currently at risk (again, until 5.1 is released)
--
Ticket URL: <https://code.djangoproject.com/ticket/34436#comment:2>
Django
warnings
-------------------------------+--------------------------------------
Reporter: James Addison | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by Mariusz Felisiak):
`check` and `makemigrations --check` are completely different commands:
- `check` is to run system checks
- `makemigrations --check` exits with a non-zero status when model changes
without migrations are detected, so the `--check` option has nothing to do
with system check.
> the 'check' for missing migrations yielded no missing migrations, so
should not return an error.
`makemigrations --check` no longer creates missing migrations files in
Django 4.2+ (see 80d38de52bb2721a7b44fce4057bcff571afc23a and #34051).
--
Ticket URL: <https://code.djangoproject.com/ticket/34436#comment:3>
Django
warnings
-------------------------------+--------------------------------------
Reporter: James Addison | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by James Addison):
To close this off - while I don't think we were talking the same language
in the conversation back and forth here, this was very much a red
herring/rabbit hole. For the benefit of others who might encounter this
behaviour, it's worthwhile adding the following.
`manage.py makemigrations --check` was ''correctly'' returning an exit
code of `1` because of the
[https://docs.djangoproject.com/en/4.1/releases/4.0/#migrations-
autodetector-changes migration autodetector changes in Django 4.0].
This manifested somewhat indirectly in a [https://github.com/python-
social-auth/social-app-django/issues/429#issuecomment-1485542714 3rd party
app], which is where the missing migration (although a no-op migration!)
needs to be added. Until that is done in 3rd party apps, continuous
integration relying on `makemigrations --check` will continue to fail.
--
Ticket URL: <https://code.djangoproject.com/ticket/34436#comment:4>