Re: makemigrations --exit; exit status feels backwards from conventions

[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

On Fri, Oct 23, 2015 at 7:33 AM, Chris Foresman  wrote:
> Jon,
>
> You proposal seems like a sane and welcome change that aligns the exit
> status of --exit with long-standing convention.
>
>
> Thanks,
> Chris
>
>
>
> On Wednesday, October 21, 2015 at 10:20:09 AM UTC-5, Jon Dufresne wrote:
>>
>> On Tue, Oct 20, 2015 at 9:29 PM, Gavin Wahl  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
>
> --
> 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-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@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.
>
> For more options, visit https://groups.google.com/d/optout.

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/CADhq2b7ajz50vU_NEpwmQ8F8%3DmyVo-pKGoOkTDYbL90LrN7tYg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: makemigrations --exit; exit status feels backwards from conventions

[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



On Wednesday, October 21, 2015 at 10:20:09 AM UTC-5, Jon Dufresne wrote:
>
> On Tue, Oct 20, 2015 at 9:29 PM, Gavin Wahl  > 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 
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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.
For more options, visit https://groups.google.com/d/optout.

Re: makemigrations --exit; exit status feels backwards from conventions

[Jon Dufresne]

On Tue, Oct 20, 2015 at 9:29 PM, Gavin Wahl  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

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/CADhq2b4N24e1TipB-DPCRZxBk71JW0NEs6F_8gKSLDk_ZtQ-tg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: makemigrations --exit; exit status feels backwards from conventions

[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

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/61887548-63b9-4a45-b1f3-689650e84154%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: makemigrations --exit; exit status feels backwards from conventions

[Jon Dufresne]

On Tue, Oct 20, 2015 at 1:48 PM, Jon Dufresne  wrote:
> On Tue, Oct 20, 2015 at 1:18 PM, Shai Berger  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.

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

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/CADhq2b47ioAcHa9wB0baV1KNGaO3uOdGhw68Pt5S4s1Y-GZhZQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: makemigrations --exit; exit status feels backwards from conventions

[Jon Dufresne]

On Tue, Oct 20, 2015 at 1:18 PM, Shai Berger  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

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/CADhq2b79dkVyXPhzVYieSjo%2BTQWs%3DhixqYRpAv539B1YzM3q7g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: makemigrations --exit; exit status feels backwards from conventions

[Shai Berger]

On Tuesday 20 October 2015 20:09:36 I wrote:
> 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  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".
> 

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

Re: makemigrations --exit; exit status feels backwards from conventions

[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  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.

Re: makemigrations --exit; exit status feels backwards from conventions

[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.

On 20 October 2015 at 05:20, Jon Dufresne  wrote:

> 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
>
> --
> 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-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@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.
>

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group

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
.

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

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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.