makemigrations --exit; exit status feels backwards from conventions

[Jon Dufresne]

Before posting this, I've read through the full thread "sys.exit(1)
from makemigrations if no changes found" at
<https://groups.google.com/d/topic/django-developers/I3M6B-wYYd8/discussion>.

I fully agree with the spirit of the change. I already find the
feature useful in CI. However, after using this feature on a CI
server, I find the exit status backwards compared to typical commands.
The makemigrations command returns status 0 to indicate CI failure
(migrations missing) and 1 to indicate CI pass (continue to the next
CI stage).

Typically status 0 indicates pass and non-zero indicates failure. By
following the typical exit status conventions, commands can explicitly
return a non-zero status when detecting a failure or the Python
runtime may return a non-zero status if something goes terribly wrong;
such as an unhandled exception.

Someone that is accustomed to typical exit status conventions might
naively use the makemigrations command:

./manage.py makemigrations --dry-run --exit

The expectation: the next stage should continue if there are no new
migrations (the developer did everything they were supposed to do and
included migrations). However, the above command will return status 1,
not 0, if there are no new migrations.

OK, we can test for that. Maybe change the command to:

! ./manage.py makemigrations --dry-run --exit

That is, interpret the exit status opposite of what one would
typically expect. Immediately, this looks backwards compared to
typical shell scripting. But what happened to the "terribly wrong"
scenario? For example, what if a developer mistakenly added an
incorrect setting that caused an ImproperlyConfigured error? If this
were to happen, I would want the above command to fail and stop the CI
pipeline.

So maybe the next step would be to check explicitly for exit code 1.

./manage.py makemigrations --dry-run --exit; test $? -eq 1

Now it looks like we're hacking around something.

Additionally, Python exits with status 1 for unhandled exceptions. So
the above command would still pass the CI stage for an unhandled
exception. Further Django's BaseCommand.run_from_argv() also exits
with status code 1 on CommandError, so again, it would pass the CI
stage for anything that triggers this sort of error.

It seems exit status 1 is overloaded to mean "all migrations are
accounted for, continue to the next stage of CI", and "something went
terribly wrong".

This is why I feel the exit status is backwards from what is typically expected.

I would like to suggest we find a better way to interface with CI
servers. That is return 0 when there are no migrations (continue to
the next stage) and non-zero for both "migrations missing" and
"something went terribly wrong".

I suggest maybe adding a system check for missing migrations. The
check could report an error, when they are missing. The check
framework seems like a natural command to be used by CI servers
anyway, so this seems like a good place. The missing migration
detection already exists, so the same code could be leveraged for this
check.

I'm also open to other suggestions on creating a more convention exit status.

If there is agreement and the proposal sounds good, I can follow
through with a ticket and code changes.

Cheers,
Jon

[Marc Tamlyn]

I see what you mean and the inherent problems with the design. However fundamentally the command you are running is "make some migrations", and the "I don't have any to make" step is clearly an error case, not a success case. In particular it would be very wrong for the real "I want to make some migrations" command to return a non-zero code when it does successfully create some.

The only real solution I can see is a separate command, or something like makemigrations --check.

--
You received this message because you are subscribed to the Google Groups "Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CADhq2b5YDr-HB5sUdwKK-K2awQZk7qUhJJdaU%2B4SH_6nx9x%3D5w%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Shai Berger]

On Tuesday 20 October 2015 19:51:36 Marc Tamlyn wrote:
> I see what you mean and the inherent problems with the design. However
> fundamentally the command you are running is "make some migrations", and
> the "I don't have any to make" step is clearly an error case, not a success
> case. In particular it would be very wrong for the real "I want to make
> some migrations" command to return a non-zero code when it does
> successfully create some.
>
> The only real solution I can see is a separate command, or something like
> makemigrations --check.

Agreed. In particular,

> On 20 October 2015 at 05:20, Jon Dufresne <jon.du...@gmail.com> wrote:
> >
> > It seems exit status 1 is overloaded to mean "all migrations are
> > accounted for, continue to the next stage of CI", and "something went
> > terribly wrong".
> >

No; it seems the "makemigrations" command is overloaded to mean both "make
migrations for me" and "verify that no migrations need to be made".

HTH,
Shai.

[Shai Berger]

On second thought, I take that back.

As is evident from the discussion Jon cited[1], the --exit flag was intended to
be used as Marc's --check. Jon is basically correct in his analysis, and I
think the root issue is that "--exit" is just a bad name for "--check".

[1] https://groups.google.com/d/topic/django-developers/I3M6B-wYYd8/discussion

[Jon Dufresne]

On Tue, Oct 20, 2015 at 1:18 PM, Shai Berger <sh...@platonix.com> wrote:
> On second thought, I take that back.
>
> As is evident from the discussion Jon cited[1], the --exit flag was intended to
> be used as Marc's --check. Jon is basically correct in his analysis, and I
> think the root issue is that "--exit" is just a bad name for "--check".
>
> [1] https://groups.google.com/d/topic/django-developers/I3M6B-wYYd8/discussion

How would you feel about putting "--exit" on a deprecation path and
replacing it with a "--check" flag? The "--check" flag would return 0
if no migrations are missing and non-zero if they are missing.

Cheers,
Jon

[Jon Dufresne]

To get a sense of what this would mean, I've created a POC with this
idea at: https://github.com/django/django/pull/5453

If the idea and design is well received, I'll create a ticket and
continue with it.

Thanks,
Jon

[Gavin Wahl]

In your case, successfully creating a migration indicates a failure. Why do you object to using a ! to communicate this?

! ./manage.py makemigrations

[Jon Dufresne]

On Tue, Oct 20, 2015 at 9:29 PM, Gavin Wahl <gavi...@gmail.com> wrote:
> In your case, successfully creating a migration indicates a failure.

Only if the --check flag is on. The --check flag indicates that one is
explicitly checking that all model changes have migrations. A non-zero
exist status indicates that migrations were missing. If you feel the
help text could be improved to communicate this, I can work towards
that.

> Why do you object to using a ! to communicate this?

With the --check flag or the --exit flag?

I think I covered this in the OP. But just to clarify:

My use case:

Continuous integration server check's developers' commits for
correctness. One aspect of correctness is that all model changes have
migrations.

In shell scripting and CI servers an exit status of 0 indicates
true/pass and an exit status of non-zero indicates false/failure.

Therefore, the command should return 0 when everything is OK and
correct and non-zero otherwise. Commits are correct when all model
changes are accounted for.

The current --exit behavior, returns non-zero when everything is
correct. To account for this in CI, one must negate the exit status
with !, this goes against conventional behavior.

Further, if something goes terribly wrong and there is an unhandled
exception, negating the exit status will make the CI stage appear to
pass. This is backwards! CI can't tell the difference between "all
changes accounted for" and "Python had an unhandled exception".


Cheers,
Jon

[Chris Foresman]

Jon,

You proposal seems like a sane and welcome change that aligns the exit status of --exit with long-standing convention.

Thanks,

Chris

[Jon Dufresne]

Thanks everyone for the useful feedback. I've created a ticket where
the development of the idea can continue:
https://code.djangoproject.com/ticket/25604

> --
> You received this message because you are subscribed to the Google Groups
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/django-developers/4a641f83-2334-4f64-b9d5-438c094dbe27%40googlegroups.com.