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.

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

adding verbose output for statement execution #138

Merged
merged 6 commits into from Jun 25, 2020

Conversation

vperiyasamy
Copy link

Adding a verbose command line flag to allow users to print the result of each statement execution. Should not disrupt the existing flow for users as it's completely optional. Summary of changes:

  • new verbose flag
  • new field for db to print result of migrations
  • new interface methods for drivers to pass verbose flag
  • capture and print of result for every exec based on verbose flag across db and drivers

@vperiyasamy
Copy link
Author

changed name from --verbose because urfave cli was panicking at redefining the flag v. new flag is now called print-result

@vperiyasamy
Copy link
Author

@amacneil - not sure how to assign reviewers

@amacneil
Copy link
Owner

amacneil commented Jun 3, 2020

Hey, sorry for the delay on this, it has been a busy few days.

Some quick thoughts/feedback:

  • I think it would make more sense for the flag to exist on the command, rather than globally (it's a urfave/cli thing). So this would make the argument dbmate up --verbose rather than dbmate --verbose up. I assume that might also fix your problem with overloading the -v shorthand.
  • Any thoughts on how to test this?
  • What does the output look like? Is it worth formatting that result struct to make it prettier?

Otherwise looks good, thanks for the contribution!

@vperiyasamy
Copy link
Author

thanks for the comments, no worries. I think I've addressed these concerns with the new commit:

  • moved flag to subcommands for up, migrate, and rollback - you were right, I am able to use verbose now
  • I added a test in db_test.go which captures the stdout and makes sure it reports the Rows affected value
  • output is a formatted string I've created, you can see it in utils.go
  • originally in this PR I had added verbose statements to all of the driver execution results, but after testing I found out that these have no effect because there is no InsertId or RowsAffected for those commands, so have removed those 👍

@vperiyasamy
Copy link
Author

@amacneil bump

@vperiyasamy
Copy link
Author

@amacneil sorry for the pestering, but any chance you would be able to look at this soon? we feel that without this logging we are missing a crucial sanity check when running our db migrations at scale

@amacneil
Copy link
Owner

Cool, this looks good. Sorry I have been busy lately and not much time for OSS. I added you as a contributor, so if you're happy with it and it's passing CI feel free to merge!

@vperiyasamy
Copy link
Author

@amacneil thank you! no worries, I appreciate it.

@vperiyasamy vperiyasamy merged commit 24705c5 into amacneil:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants