Automattic / vip-cli

The VIP-CLI
https://docs.wpvip.com/vip-cli/
MIT License
58 stars 17 forks source link

Fix `--output` option of `vip import sql` command #2056

Closed mehmoodak closed 1 month ago

mehmoodak commented 1 month ago

Description

Fixed an issue with the output option of the vip import sql command, which wasn't creating an output file as expected. Alongside this fix, this PR also improves the help text for the vip search-replace command.

Pull request checklist

New release checklist

Steps to Test

  1. Check out PR.
  2. Run vip @example-app.develop export sql --output=output.sql.gz
  3. Extracts the sql file from gzip.
  4. Run node ./dist/bin/vip-import-sql.js @example-app.develop output.sql --search-replace="from,to" --output="test-output.sql"
  5. Verify test-output.sql file is created with the updated content.
github-actions[bot] commented 1 month ago

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

mehmoodak commented 1 month ago

Would it be possible for your PR to only fix the --output bug and for my PR to make the updates to the --help menu?

@yolih I have made some changes, please take a look again.

yolih commented 1 month ago

Thanks, @mehmoodak ! 🙌

A couple more questions:

mehmoodak commented 1 month ago

@yolih The key difference is that vip import sql requires a SQL file to be uploaded to S3, and printing to stdout will result in an error. In contrast, vip search-replace can use stdout without any issues.

yolih commented 1 month ago

Oooohhhh... interesting. Thank you for your patience with all of my questions!

Based on your explanation, I agree with removing Defaults to STDOUT from the vip import sql version of --output.

For the vip search-replace version of --output, I suggest that the description in the help menu reads something like:

Create a local copy of the file with the completed search and replace operations. Ignored if the command includes --in-place. Accepts a local file path. Defaults to STDOUT.

What do you think?

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mehmoodak commented 1 month ago

What do you think?

@yolih help text updated. Thanks for helping me improve it.