Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return exit code from alembic #8

Open
trilader opened this issue Sep 29, 2021 · 6 comments · May be fixed by #9
Open

Return exit code from alembic #8

trilader opened this issue Sep 29, 2021 · 6 comments · May be fixed by #9
Labels
bug Something isn't working

Comments

@trilader
Copy link

I had an error in a migration and I noticed that Flask-DB discards the exit code from alembic.

You can test this easily by putting, e.g., raise ValueError("This should fail") into a migrations upgrade method. When running alembic upgrade head directly the process exits with 1. When running flask db migrate upgrade head the exception gets printed but the commands exit code is set to 0. This is an issue when running database migrations with a configuration management system, in my case Ansible, as the error will silently be ignored.
@nickjj
Copy link
Owner

nickjj commented Sep 29, 2021 •
edited
Loading

Thanks for the report.

It looks like in this function:

flask-db/flask_db/cli.py

Lines 120 to 129 in 109199f

def migrate(alembic_args):
"""Wrap the alembic CLI tool (defaults to running upgrade head)."""
if not alembic_args:
alembic_args = ("upgrade", "head")
cmdline = ["alembic"] + list(alembic_args)
subprocess.call(cmdline)
return None

If we replaced return None with sys.exit(subprocess.call(cmdline)) it should properly return the exit code. Would you mind giving that a test run? You'll also need to import sys to make that work.

@nickjj nickjj added the bug Something isn't working label Sep 29, 2021
@trilader
Copy link
Author

I just tested your suggestion and it works as expected. Do you want a pull request for this?

@nickjj
Copy link
Owner

nickjj commented Sep 29, 2021

A PR would be much appreciated but I think if we do this we should probably adjust all commands to throw a proper exit code. Would you mind testing them to see if it works without any changes since they use a different method?

The seed command is at:

flask-db/flask_db/cli.py

Line 101 in 109199f

exec(open(seeds_path).read())

We might need to wrap that in a sys.exit() too.

There's also the reset command too:

flask-db/flask_db/cli.py

Line 86 in 109199f

ctx.invoke(seed)

I'm not sure if this carries over the exit code too.

@trilader
Copy link
Author

For the reset command it definitely looks like a good idea. The docs say invoke returns a result object which includes the exit_code so we can sys.exit with that. In my simple case it does what I expect.

Regarding the seed command: As far as I know exec doesn't return anything by itself but it just executes the string or file passed to it. If that has a sys.exit in it, it will exit Flask-DB with whatever it gets passed as well. But we can't know what the user will put into their seed file so we can't rely on that.

trilader added a commit to trilader/flask-db that referenced this issue Sep 29, 2021
@trilader
Exit with child exit codes for reset and migrate commands
10150e2 
These commands are basically wrappers for the seed subcommand and alembic
itself. They have an exit code which we now forward to the user calling us.

Fixes nickjj#8
@trilader trilader linked a pull request Sep 29, 2021 that will close this issue
Open
@nickjj
Copy link
Owner

nickjj commented Sep 29, 2021 •
edited
Loading

Hmm for the seed command what if we wrapped the exec() call in a try / except and on any error then we sys.exit(1)?

@mzhukovs
Copy link

Any updates on this? Ran into the same issue when trying to set up running migrations as part of an automated Azure DevOps pipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Exit with child exit codes for reset and migrate commands trilader/flask-db
3 participants
@nickjj @trilader @mzhukovs