-
Notifications
You must be signed in to change notification settings - Fork 92
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
feature/sc-126217/implement-update-cli-command-in-js #729
feature/sc-126217/implement-update-cli-command-in-js #729
Conversation
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.
Looks good generally. Additional tweaks may be necessary before final release but you can merge this
if (skip) { | ||
return; | ||
} | ||
|
||
const now = Date.now(); | ||
const lastCheck = settings.profile_json.last_version_check || 0; | ||
const skipUpdates = !settings.profile_json.enableUpdates || settings.disableUpdateCheck; |
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.
If someone doesn't set enableUpdates
it will skip updates. I think that's ok if we assume most people will install through the installer. npm install wouldn't set enableUpdates to true so that's fine. You may need to add a call to particle update-cli --enable-updates
when installing from Workbench.
How about going back to the PRD and putting a little table of which installation methods enable updates, which ones don't and which ones have an option to enable/disable updates.
src/app/update-check.js
Outdated
return; | ||
} | ||
return; | ||
execa('particle', ['update-cli']); |
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.
You probably need some additional options for execa. Specifically, look at cleanup and detached to avoid the background process being killed when the current process exits.
https://www.npmjs.com/package/execa#cleanup
https://www.npmjs.com/package/execa#detached
} | ||
if (!dirPath.includes('snapshot')) { | ||
log.info(`Update the CLI by running ${chalk.bold('npm install -g particle-cli')}`); | ||
log.info('To stay up to date with the latest features and improvements, please install the latest Particle Installer executable from our website: https://www.particle.io/cli'); |
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.
Good message!
src/cmd/update-cli.js
Outdated
log.info('Automatic update checks are now disabled'); | ||
return this.disableUpdates(); | ||
} | ||
if (!dirPath.includes('snapshot')) { |
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.
Did you try process.pkg
to detect whether it's running under pkg?
src/cmd/update-cli.js
Outdated
const log = require('../lib/log'); | ||
const chalk = require('chalk'); | ||
const settings = require('../../settings'); | ||
const MANIFEST_HOST = process.env.PARTICLE_MANIFEST_HOST || 'binaries.particle.io'; |
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.
This would seem better in settings.js
src/cmd/update-cli.js
Outdated
resolve(filePath); | ||
}) | ||
.on('error', (error) => { | ||
log.error(`Error downloading CLI: ${error}`); |
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.
Did you test what the error looks like when offline or version doesn't exist?
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.
It will show a 404 or econnrefused so I'll change it like I did in the manifest => put a log.debug for the actual error and return a more helpful error to the user.
…spawn update process
Description
This PR will implement update-cli by using the new
manifest
and build structure.How to Test
git pull && git checkout feature/sc-126217/implement-update-cli-command-in-js
npm i
npm start -- update-cli
./particle -- update-cli
./particle -- update-cli --version 1.2.3
./particle -- update-cli --version 3.23.0
Outcome
npm i -g particle-cli
in order to update it~/.particle/profile.json
)Related Issues / Discussions
Story details: https://app.shortcut.com/particle/story/126217
Completeness