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

Projects
None yet
4 participants
@longwave
Copy link
Contributor

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

This comment has been minimized.

Copy link

CLAassistant commented Dec 21, 2018

CLA assistant check
All committers have signed the CLA.

@longwave

This comment has been minimized.

Copy link
Contributor Author

longwave commented Dec 21, 2018

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

This comment has been minimized.

Copy link
Member

rfay commented Dec 21, 2018

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

@longwave

This comment has been minimized.

Copy link
Contributor Author

longwave commented Dec 21, 2018

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

@rfay

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

longwave commented Dec 26, 2018

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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.

@rfay rfay force-pushed the longwave:import-db-progress branch from 4c9252f to 083e51e Jan 7, 2019

@rfay

rfay approved these changes Jan 7, 2019

Copy link
Member

rfay left a comment

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 drud:master Jan 8, 2019

8 checks passed

buildkite/ddev-containers-macos Build #1352 passed (32 minutes, 25 seconds)
Details
buildkite/ddev-containers-windows Build #1216 passed (38 minutes, 5 seconds)
Details
buildkite/ddev-macos Build #1671 passed (3 hours, 3 minutes, 5 seconds)
Details
buildkite/ddev-macos-use-webcache Build #202 passed (2 hours, 21 minutes, 21 seconds)
Details
buildkite/ddev-windows-apache-fpm Build #784 passed (2 hours, 13 minutes, 24 seconds)
Details
buildkite/ddev-windows-dockerforwindows Build #1754 passed (1 hour, 20 minutes, 38 seconds)
Details
buildkite/ddev-windows-dockertoolbox Build #1359 passed (1 hour, 17 minutes, 34 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
@rfay

This comment has been minimized.

Copy link
Member

rfay commented Jan 8, 2019

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

@rfay rfay referenced this pull request Jan 14, 2019

Closed

v1.5.2 Release Checklist Due 2019-01-15 #1343

6 of 8 tasks complete

@dclear dclear added this to the v1.5.2 milestone Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment