Migration failure needs to be obvious and clean

[steder]

Currently migration failures are obvious because we just throw the sqlalchemy.exc.OperationalError exception and the big ol' most irrelevant stack trace.

The user needs to know what sql it was trying to run that failed so that they can go fix it.

The current behavior is correct because it's a obvious failure and it stops the migration run.

However, I think that throwing a stack trace at the user when it is not a Python problem is inappropriate and misleading. The real problem in this particular case is with their SQL. I'd prefer someone only see a stack trace from Goose when they've encountered a bug in the program.

If Goose is used as a library then it's perfectly reasonable to catch and rethrow the exception but as a commandline tool it should behave, display an appropriate error message and exit with a non-zero status code.

A python script that exits with an uncaught exception will return status code 1 appropriately indicating failure.

The current stack trace is:

Traceback (most recent call last): File "./bin/goose", line 5, in core.main(sys.argv[1:]) File "/Users/steder/Sites/Goose/goose/core.py", line 283, in main init=init) File "/Users/steder/Sites/Goose/goose/core.py", line 249, in migrate migrator.migrate(fromVersion, toVersion, selectedMigrations) File "/Users/steder/Sites/Goose/goose/core.py", line 190, in migrate rval = self.runSql(migration, version) File "/Users/steder/Sites/Goose/goose/core.py", line 162, in runSql executeBatch(self.session, sql) File "/Users/steder/Sites/Goose/goose/core.py", line 44, in executeBatch cursor.execute(st) File "/Users/steder/VirtualEnvs/goose/lib/python2.7/site-packages/SQLAlchemy-0.7.5-py2.7-macosx-10.4-x86_64.egg/sqlalchemy/orm/session.py", line 801, in execute File "/Users/steder/VirtualEnvs/goose/lib/python2.7/site-packages/SQLAlchemy-0.7.5-py2.7-macosx-10.4-x86_64.egg/sqlalchemy/engine/base.py", line 1405, in execute File "/Users/steder/VirtualEnvs/goose/lib/python2.7/site-packages/SQLAlchemy-0.7.5-py2.7-macosx-10.4-x86_64.egg/sqlalchemy/engine/base.py", line 1538, in _execute_clauseelement File "/Users/steder/VirtualEnvs/goose/lib/python2.7/site-packages/SQLAlchemy-0.7.5-py2.7-macosx-10.4-x86_64.egg/sqlalchemy/engine/base.py", line 1646, in _execute_context File "/Users/steder/VirtualEnvs/goose/lib/python2.7/site-packages/SQLAlchemy-0.7.5-py2.7-macosx-10.4-x86_64.egg/sqlalchemy/engine/base.py", line 1639, in _execute_context File "/Users/steder/VirtualEnvs/goose/lib/python2.7/site-packages/SQLAlchemy-0.7.5-py2.7-macosx-10.4-x86_64.egg/sqlalchemy/engine/default.py", line 330, in do_execute sqlalchemy.exc.OperationalError: (OperationalError) near "FRO": syntax error u'\nSELECT * FRO Track' ()

Modifying that to re-raise a Goose exception: (I'm just using Exception for expediency) still presents the user with:

Traceback (most recent call last): File "./bin/goose", line 5, in core.main(sys.argv[1:]) File "/Users/steder/Sites/Goose/goose/core.py", line 283, in main init=init) File "/Users/steder/Sites/Goose/goose/core.py", line 249, in migrate migrator.migrate(fromVersion, toVersion, selectedMigrations) File "/Users/steder/Sites/Goose/goose/core.py", line 190, in migrate rval = self.runSql(migration, version) File "/Users/steder/Sites/Goose/goose/core.py", line 170, in runSql raise Exception("%s"%(e,)) Exception: (OperationalError) near "FRO": syntax error u'\nSELECT * FRO Track' ()

I'm proposing the output look like:

Error occured running migration: bad.sql (OperationalError) near "FRO": syntax error (bad.sql:1)

Of course, if the script no longer exits with a traceback we need to still exit appropriately with a non-zero status code just like python would've done had we raised an exception.

[ldanielburr]

I agree with the title of this issue, but vehemently disagree that return integer codes is a good, or even decent, means of accomplishing this aim. In order to provide "an appropriate non-zero value on failure" means parsing the OperationalError anyway, so why not convert it to a goose.exceptions.SomeAppropriateException, with a "more informative and user-friendly" message attribute?

[steder]

I'm not advocating that the return code be used by people using Goose as a library within their startup process.
I'm also not thinking that they should use a return code ("Just run goose and then "echo $?" to check the return code!").

I'm just thinking that any traceback is irrelevant in the case of an error in a SQL file but the migration should still fail in a way that is unambiguous for a runner like Buildbot or Jenkins.

http://buildbot.net/buildbot/docs/0.8.5/manual/cfg-buildsteps.html

[ldanielburr]

I'm not quite with you yet, though I agree with what you said regarding buildbot/jenkins. What I don't understand is the phrase "any traceback is irrelevant in the case of an error in a SQL file"; it seems pretty relevant to me. Sure the database driver is the thing that actually raises the OperationalError, or what-have-you, but I think it is no different than raising OSError when performing a file-system operation: I still want to know why the error was raised, and the traceback will contain the actual exception message (from the rdbms in this case), and I would want to know that.

[steder]

Yeah, I see your point. On Mar 11, 2012 1:34 PM, "L. Daniel Burr" < reply@reply.github.com> wrote:

I'm not quite with you yet, though I agree with what you said regarding buildbot/jenkins. What I don't understand is the phrase "any traceback is irrelevant in the case of an error in a SQL file"; it seems pretty relevant to me. Sure the database driver is the thing that actually raises the OperationalError, or what-have-you, but I think it is no different than raising OSError when performing a file-system operation: I still want to know why the error was raised, and the traceback will contain the actual exception message (from the rdbms in this case), and I would want to know that.

---

Reply to this email directly or view it on GitHub: https://github.com/steder/goose/issues/16#issuecomment-4441892