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

Use pv to show progress bar during import-db #1349

Merged
merged 7 commits into from Jan 8, 2019

Conversation

longwave
Copy link
Contributor

@longwave longwave commented Dec 21, 2018

The Problem/Issue/Bug:

There is no feedback during long running database imports.

How this PR Solves The Problem:

This PR uses pv instead of cat to read the database dump which provides a progress bar on stderr.

Manual Testing Instructions:

  • Import a (preferably large) database dump file with ddev import-db and check that a progress bar is displayed.
  • Import a database dump with ddev import-db -p=false and check the progress bar is hidden.

Automated Testing Overview:

Changes are visual only, and require a tty, so automated testing is quite tricky.

Related Issue Link(s):

Fixes #1348

Release/Deployment notes:

Automated processes that use ddev import-db should not be affected as we detect presence of a tty and disable the progress bar in these environments. There is also the -p=false switch to disable the progress bar if needed.

@CLAassistant
Copy link

CLAassistant commented Dec 21, 2018

CLA assistant check
All committers have signed the CLA.

@longwave
Copy link
Contributor Author

The tests fail because they are not connected to a tty, so it looks like I need to use github.com/mattn/go-isatty to detect whether stdout is a tty or not, and call Exec or ExecWithTty as appropriate. This might also be a good point to add a switch to disable progress even if the tty is present.

@rfay
Copy link
Member

rfay commented Dec 21, 2018

You're awesome, thanks for the PR! Ambitious!

@longwave
Copy link
Contributor Author

A followup PR could refactor away ExecWithTty as Exec now has a tty option, and only the ssh command uses ExecWithTty.

@rfay
Copy link
Member

rfay commented Dec 26, 2018

Assuming this is just a test issue, happy to help when and if you're ready for help.

@longwave
Copy link
Contributor Author

Any review comments or help with test fixes would be appreciated. I don't have time to look at this again until next year now.

@longwave
Copy link
Contributor Author

longwave commented Jan 3, 2019

@rfay The lint and compile steps seem to be working but it looks like the CircleCI tests don't rebuild the container images, so pv is not available during the tests?

@rfay
Copy link
Member

rfay commented Jan 3, 2019

What we have to do is push a new version of the container, and update in pkg/version/version.go with the new version. I just pushed drud/ddev-dbserver:20190103_import-db-progress-10.1 and drud/ddev-dbserver:20190103_import-db-progress-10.2, please add a commit to pkg/version/version.go that updates the version there (temporarily, until next release), and then you'll get the right container.

@longwave
Copy link
Contributor Author

longwave commented Jan 3, 2019

@rfay I can't see the buildkite results so don't know why the Windows builds have failed. The dockerforwindows one is especially suspicious at only 2 seconds runtime.

@rfay
Copy link
Member

rfay commented Jan 3, 2019

I've got those tests restarted for you, looks like you're mighty close!

Thanks for the awesome work on this. I'm looking forward to taking a look at it.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased and tested this with a largish db on macOS; it worked fine with both ddev import-db and ddev pull. It may be that ddev pull needs the progressOption arg as well? But think we're good to go when it gets through the last round of tests.

@rfay rfay merged commit e559fa0 into ddev:master Jan 8, 2019
@rfay
Copy link
Member

rfay commented Jan 8, 2019

You're awesome, thanks so much for this excellent contribution @longwave - I like it!

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

4 participants