Use `--file` for import-db and rework import-db and export-db, fixes #4593 #5013

gilbertsoft · 2023-06-23T15:10:27Z

The Issue

Add "file" as alias for import-db flag "src" #4593

How This PR Solves The Issue

This PR streamlines the flags and descriptions of the import-db and export-db commands. It introduces a new package which makes it possible to write cleaner command definitions regarding indentation. Commands are created with a factory function.

Old flags are still here and marked as deprecated to avoid BC issues.

Thanks to @miromichalicka for the initial work done in #4647.

Manual Testing Instructions

The impact is minimal and everything should be covered by automated testing.

import-db command works including deprecated flags
export-db command works including deprecated flags

Automated Testing Overview

Release/Deployment Notes

github-actions · 2023-06-23T15:14:57Z

Download the artifacts for this pull request:

See Testing a PR

rfay

I don't completely grok this yet, but please update the description here to

explain the new internal/text package (and why you call it "text", which is not very descriptive)
explain what the heredoc package is and why it's used

Although this is apparently simple, it is definitely not easy to review.

Since this will apparently be a pattern for future updates, could you please remove all the var usages with pointers, and instead use flags, as is done in newer commands? start.go uses this technique, for example

ddev/cmd/ddev/cmd/start.go

Lines 36 to 47 in bbb2cac

    
           skip, err := cmd.Flags().GetBool("skip-confirmation") 
        
           if err != nil { 
        
           	util.Failed(err.Error()) 
        
           } 
        
           // Look for version change and opt-in to instrumentation if it has changed. 
        
           err = checkDdevVersionAndOptInInstrumentation(skip) 
        
           if err != nil { 
        
           	util.Failed(err.Error()) 
        
           } 
        
           selectFlag, err := cmd.Flags().GetBool("select")

gilbertsoft · 2023-06-23T19:08:29Z

I don't completely grok this yet, but please update the description here to
* [ ]  explain the new internal/text package (and why you call it "text", which is not very descriptive)

Highly inspired by GH cli. It's about text helpper functions so the name looks fine to me.

* [ ]  explain what the heredoc package is and why it's used

It's explained in the PR, it's about the heredoc syntax which allows to indent texts so we do not longer have texts starting on the first col but can be indented properly like the code.

Although this is apparently simple, it is definitely not easy to review.

Yeah, the main thing which has changed is the indentation but the diff view is not very helpful here. The biggest change is the access to the flags like requested.

Since this will apparently be a pattern for future updates, could you please remove all the var usages with pointers, and instead use flags, as is done in newer commands? start.go uses this technique, for example

Done.

docs/content/users/usage/commands.md

rfay · 2023-06-23T19:27:13Z

Just read the heredoc readme, I like that feature! Not sure why they named it "heredoc", which means something completely different in bash.

gilbertsoft · 2023-06-23T20:14:28Z

Just read the heredoc readme, I like that feature! Not sure why they named it "heredoc", which means something completely different in bash.

heredoc is a name in the PHP world, not sure if somewhere else too. See https://www.php.net/manual/en/language.types.string.php#language.types.string.syntax.heredoc

rfay

This seems like a great improvement and good example of command setup, thanks! I manually tested and everything seems to work fine.

Please don't change import-db's --target-db. There was no request for this, and there's no reason to break people's existing scripts and such.
I can cope with the way you've implemented --file but the idea was to support it as an alternate rather than as a replacement for --src. It's OK though.
Please credit @miromichalicka in the commit when this goes in.
Would you mind changing the text package to something like "indent" instead? "text" is already the name of dozens of packages.
Interested why the package doesn't just go into pkg; that would be the natural place for it. Unless you have a strong argument I'd prefer for it to be there.

cmd/ddev/cmd/import-db.go

gilbertsoft · 2023-06-24T12:58:49Z

This seems like a great improvement and good example of command setup, thanks! I manually tested and everything seems to work fine.
* Please don't change import-db's --target-db. There was no request for this, and there's no reason to break people's existing scripts and such.

Nothing will break, --target-db still exists. And I really like to have this streamlined with other tools e.g. mysqldump, that's why I've changed it in the first place. We should always try to use standards from other tools so it easier for people to use.

* I can cope with the way you've implemented `--file` but the idea was to support it as an alternate rather than as a replacement for `--src`. It's OK though.

Same as above, src still exists. But we can not use source for import because there it's the target or destination but file we can use in all places.

* Please credit @miromichalicka in the commit when this goes in.

* Would you mind changing the text package to something like "indent" instead? "text" is already the name of dozens of packages.

* Interested why the package doesn't just go into pkg; that would be the natural place for it. Unless you have a strong argument I'd prefer for it to be there.

Changed it to heredoc, best name I've found and also moved to pkg now.

rfay

No, --target-db does not work here, which is why the tests are failing. Looks like it's just a bug that makes it not work though. Please manually test.

$ ddev import-db --src=.tarballs/db.sql.gz --target-db=junk
Flag --src has been deprecated, please use --file instead
Flag --target-db has been deprecated, please use --database instead
Error: failed to import database 'db' for d9: Unable to validate import asset junk: invalid asset: file not found
Usage:
  ddev import-db [project] [flags]

Examples:
  $ ddev import-db
  $ ddev import-db --file=.tarballs/junk.sql
  $ ddev import-db --file=.tarballs/junk.sql.gz
  $ ddev import-db --database=other_db --file=.tarballs/db.sql.gz
  $ ddev import-db --file=.tarballs/db.sql.bz2
  $ ddev import-db --file=.tarballs/db.sql.xz
  $ ddev import-db < db.sql
  $ ddev import-db some-project < db.sql
  $ gzip -dc db.sql.gz | ddev import-db


Flags:
  -d, --database string       Target database to import into (default "db")
      --extract-path string   Path to extract within the archive
  -f, --file string           Path to a SQL dump file, plain text or compressed (tar/tar.gz/tgz/zip)
  -h, --help                  help for import-db
      --no-drop               Do not drop the database before importing
      --no-progress           Do not output progress

Global Flags:
  -j, --json-output   If true, user-oriented output will be in JSON format.

gilbertsoft · 2023-06-25T13:06:12Z

No, --target-db does not work here, which is why the tests are failing. Looks like it's just a bug that makes it not work though. Please manually test.

$ ddev import-db --src=.tarballs/db.sql.gz --target-db=junk
Flag --src has been deprecated, please use --file instead
Flag --target-db has been deprecated, please use --database instead
Error: failed to import database 'db' for d9: Unable to validate import asset junk: invalid asset: file not found
Usage:
  ddev import-db [project] [flags]

Examples:
  $ ddev import-db
  $ ddev import-db --file=.tarballs/junk.sql
  $ ddev import-db --file=.tarballs/junk.sql.gz
  $ ddev import-db --database=other_db --file=.tarballs/db.sql.gz
  $ ddev import-db --file=.tarballs/db.sql.bz2
  $ ddev import-db --file=.tarballs/db.sql.xz
  $ ddev import-db < db.sql
  $ ddev import-db some-project < db.sql
  $ gzip -dc db.sql.gz | ddev import-db


Flags:
  -d, --database string       Target database to import into (default "db")
      --extract-path string   Path to extract within the archive
  -f, --file string           Path to a SQL dump file, plain text or compressed (tar/tar.gz/tgz/zip)
  -h, --help                  help for import-db
      --no-drop               Do not drop the database before importing
      --no-progress           Do not output progress

Global Flags:
  -j, --json-output   If true, user-oriented output will be in JSON format.

Looks like a bug while reading the flags. Will have a look....

gilbertsoft · 2023-06-25T13:53:10Z

Found the problem, copy&paste issue. Same in the db-export :(

rfay · 2023-06-26T17:01:02Z

Did you intentionally re-add the -z flag? It's still there...

$ ddev --version
ddev version v1.22.0-alpha3-10-g0069dfdba
$ ddev help export-db
Dump a database to a file or to stdout.

Usage:
  ddev export-db [project] [flags]

Examples:
  $ ddev export-db --file=/tmp/db.sql.gz
  $ ddev export-db -f /tmp/db.sql.gz
  $ ddev export-db --gzip=false --file /tmp/db.sql
  $ ddev export-db > /tmp/db.sql.gz
  $ ddev export-db --gzip=false > /tmp/db.sql
  $ ddev export-db --database=additional_db --file=.tarballs/additional_db.sql.gz
  $ ddev export-db my-project --gzip=false --file=/tmp/my_project.sql


Flags:
      --bzip2             Use bzip2 compression
  -d, --database string   Target database to export from (default "db")
  -f, --file string       Path to a SQL dump file to export to
  -z, --gzip              Use gzip compression (default true)
  -h, --help              help for export-db
      --xz                Use xz compression

Global Flags:
  -j, --json-output   If true, user-oriented output will be in JSON format.
rfay@rfay-tag1-m1:~/workspace/d9$ ddev export-db -z --file .tarballs/junk2.sql.gz
Wrote database dump from project 'd9' database 'db' to file .tarballs/junk2.sql.gz in gzip format.

rfay

~~As far as I can tell, --target-db still doesn't work:~~

Edit: I messed up the command, it's working

rfay

Looks like it's ready to go, thanks! Manually tested as well.

gilbertsoft changed the title ~~Streamline flags of import- and export-db commands, fixes 4593~~ Streamline flags of import- and export-db commands, fixes #4593 Jun 23, 2023

gilbertsoft marked this pull request as ready for review June 23, 2023 18:00

gilbertsoft requested review from a team as code owners June 23, 2023 18:00

rfay requested changes Jun 23, 2023

View reviewed changes

gilbertsoft force-pushed the task/streamline-file-flags branch 2 times, most recently from ec12f71 to e6bce72 Compare June 23, 2023 18:56

mattstein requested changes Jun 23, 2023

View reviewed changes

docs/content/users/usage/commands.md Outdated Show resolved Hide resolved

docs/content/users/usage/commands.md Outdated Show resolved Hide resolved

docs/content/users/usage/commands.md Outdated Show resolved Hide resolved

docs/content/users/usage/commands.md Outdated Show resolved Hide resolved

rfay requested changes Jun 23, 2023

View reviewed changes

cmd/ddev/cmd/import-db.go Show resolved Hide resolved

rfay mentioned this pull request Jun 23, 2023

Add file as alias for "src" flag (fixes #4593) #4647

Closed

rfay changed the title ~~Streamline flags of import- and export-db commands, fixes #4593~~ Streamline flags of import- and export-db commands, use --file for import-db, fixes #4593 Jun 23, 2023

rfay changed the title ~~Streamline flags of import- and export-db commands, use --file for import-db, fixes #4593~~ Use --file for import-db and rework import-db and export-db, fixes #4593 Jun 23, 2023

gilbertsoft force-pushed the task/streamline-file-flags branch from e6bce72 to 91982d4 Compare June 24, 2023 13:04

rfay requested changes Jun 24, 2023

View reviewed changes

gilbertsoft force-pushed the task/streamline-file-flags branch from 91982d4 to 330762f Compare June 25, 2023 13:56

Streamline flags of import- and export-db commands, fixes 4593

54d62ea

gilbertsoft force-pushed the task/streamline-file-flags branch from 330762f to 54d62ea Compare June 25, 2023 13:56

gilbertsoft added 2 commits June 25, 2023 21:20

Docs changes

86ffb9f

Docs changes

c9d299f

rfay requested changes Jun 26, 2023

View reviewed changes

rfay approved these changes Jun 26, 2023

View reviewed changes

rfay merged commit 2d481a4 into ddev:master Jun 26, 2023
17 checks passed

rfay deleted the task/streamline-file-flags branch June 26, 2023 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `--file` for import-db and rework import-db and export-db, fixes #4593 #5013

Use `--file` for import-db and rework import-db and export-db, fixes #4593 #5013

gilbertsoft commented Jun 23, 2023 •

edited

github-actions bot commented Jun 23, 2023 •

edited

rfay left a comment •

edited

gilbertsoft commented Jun 23, 2023

rfay commented Jun 23, 2023

gilbertsoft commented Jun 23, 2023

rfay left a comment •

edited

gilbertsoft commented Jun 24, 2023

rfay left a comment

gilbertsoft commented Jun 25, 2023

gilbertsoft commented Jun 25, 2023 •

edited

rfay commented Jun 26, 2023 •

edited

rfay left a comment •

edited

rfay left a comment

	skip, err := cmd.Flags().GetBool("skip-confirmation")
	if err != nil {
	util.Failed(err.Error())
	}

	// Look for version change and opt-in to instrumentation if it has changed.
	err = checkDdevVersionAndOptInInstrumentation(skip)
	if err != nil {
	util.Failed(err.Error())
	}

	selectFlag, err := cmd.Flags().GetBool("select")

Use --file for import-db and rework import-db and export-db, fixes #4593 #5013

Use --file for import-db and rework import-db and export-db, fixes #4593 #5013

Conversation

gilbertsoft commented Jun 23, 2023 • edited

The Issue

How This PR Solves The Issue

Manual Testing Instructions

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

github-actions bot commented Jun 23, 2023 • edited

rfay left a comment • edited

Choose a reason for hiding this comment

gilbertsoft commented Jun 23, 2023

rfay commented Jun 23, 2023

gilbertsoft commented Jun 23, 2023

rfay left a comment • edited

Choose a reason for hiding this comment

gilbertsoft commented Jun 24, 2023

rfay left a comment

Choose a reason for hiding this comment

gilbertsoft commented Jun 25, 2023

gilbertsoft commented Jun 25, 2023 • edited

rfay commented Jun 26, 2023 • edited

rfay left a comment • edited

Choose a reason for hiding this comment

rfay left a comment

Choose a reason for hiding this comment

Use `--file` for import-db and rework import-db and export-db, fixes #4593 #5013

Use `--file` for import-db and rework import-db and export-db, fixes #4593 #5013

gilbertsoft commented Jun 23, 2023 •

edited

github-actions bot commented Jun 23, 2023 •

edited

rfay left a comment •

edited

rfay left a comment •

edited

gilbertsoft commented Jun 25, 2023 •

edited

rfay commented Jun 26, 2023 •

edited

rfay left a comment •

edited