-
Notifications
You must be signed in to change notification settings - Fork 54
CLI: Refine site stop --all implementation
#2374
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
CLI: Refine site stop --all implementation
#2374
Conversation
bcotrim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and works as described 👍
Added a couple of smaller comments, but no blockers
cli/commands/site/stop.ts
Outdated
| if ( ! runningProcess ) { | ||
| logger.reportSuccess( __( 'WordPress site is not running' ) ); | ||
| return; | ||
| const SITES_TO_TARGET: SiteData[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should we rename it to sitesToTarget?
cli/commands/site/stop.ts
Outdated
| ); | ||
| } else { | ||
| throw new LoggerError( | ||
| sprintf( __( 'Stopped %d sites out of %d' ), stoppedSiteIds.length, SITES_TO_TARGET.length ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should we handle single site error message differently?
Stopped 0 sites out of 1 seems strange.
gcsecsey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes LGTM, and everything worked well while testing. 👍
I added the two sites:
gcsecsey@Mac-Studio studio % node dist/cli/main.js site list
✔ Found 2 sites
✔ Connected to process daemon
┌─────────────┬──────────────────────────┬────────────────────────────────────────┬─────────────────────────────────────────────────────┐
│ Status │ Name │ Path │ URL │
├─────────────┼──────────────────────────┼────────────────────────────────────────┼─────────────────────────────────────────────────────┤
│ 🟢 Online │ site-1 │ ~/Studio/site-1 │ http://localhost:8881 │
├─────────────┼──────────────────────────┼────────────────────────────────────────┼─────────────────────────────────────────────────────┤
│ 🟢 Online │ site-2 │ ~/Studio/site-2 │ http://localhost:8882 │
└─────────────┴──────────────────────────┴────────────────────────────────────────┴─────────────────────────────────────────────────────┘
Stopping all:
gcsecsey@Mac-Studio studio % node dist/cli/main.js site stop --all
✔ Successfully stopped 2 sites
Running the stop --all command again:
gcsecsey@Mac-Studio studio % node dist/cli/main.js site stop --all
✔ No sites are currently running
Stopping a single site:
gcsecsey@Mac-Studio studio % node dist/cli/main.js site stop --path ~/Studio/site-2
✔ Successfully stopped 1 site
Checking the list:
gcsecsey@Mac-Studio studio % node dist/cli/main.js site list
✔ Found 2 sites
✔ Connected to process daemon
┌─────────────┬──────────────────────────┬────────────────────────────────────────┬─────────────────────────────────────────────────────┐
│ Status │ Name │ Path │ URL │
├─────────────┼──────────────────────────┼────────────────────────────────────────┼─────────────────────────────────────────────────────┤
│ 🔴 Offline │ site-1 │ ~/Studio/site-1 │ http://localhost:8881 │
├─────────────┼──────────────────────────┼────────────────────────────────────────┼─────────────────────────────────────────────────────┤
│ 🔴 Offline │ site-2 │ ~/Studio/site-2 │ http://localhost:8882 │
└─────────────┴──────────────────────────┴────────────────────────────────────────┴─────────────────────────────────────────────────────┘
Related issues
Proposed Changes
runCommandfunction instead ofrunCommandandrunCommandAllsite stop --allcommand from Studio codebase instead ofsite stop-allTesting Instructions
npm run cli:buildnode dist/cli/main.js site createto create at least two sitesnode dist/cli/main.js site stop --allnode dist/cli/main.js site stop --allNo sites are currently runningis displayednode dist/cli/main.js site stop --path PATH_TO_SITEnode dist/cli/main.js site listPre-merge Checklist