Skip to content
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

feat: add update and rollback commands, and cli new version update core plugin #132

Merged
merged 5 commits into from
Apr 2, 2020
Merged

Conversation

shazron
Copy link
Member

@shazron shazron commented Mar 30, 2020

Description

The update command will update all installed plugins by checking for any updated versions in the npm registry (with --interactive mode available). Note that this will not check for any updates for any non @adobe namespaced core plugins, for compatibility reasons.

The rollback command will uninstall all user-installed plugins (with --interactive mode available).

Through a hook via the @oclif/plugin-warn-if-update-available plugin, npm is checked whether there is a newer version of the CLI available, and it will warn the user. This is shown at every command (can't turn it off) but it has a timeout of 7 days (so after 7 days of warnings, it won't prompt the user anymore).

Motivation and Context

  1. Update of installed plugins (including core plugins) without a CLI reinstall
  2. Reminder to user that a newer cli version is available

How Has This Been Tested?

npm test
manual tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #132 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #132    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            3         5     +2     
  Lines           69       180   +111     
  Branches        10        25    +15     
==========================================
+ Hits            69       180   +111     
Impacted Files Coverage Δ
src/commands/rollback.js 100.00% <100.00%> (ø)
src/commands/update.js 100.00% <100.00%> (ø)
src/helpers.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07ce9cc...d538097. Read the comment docs.

The update command will update all installed plugins by checking for any updated versions in the npm registry (with --interactive mode available).
The clear command will uninstall all user-installed plugins (with --interactive mode available). This command is also aliased to `plugins:clear`

100% coverage on tests
@shazron shazron marked this pull request as ready for review March 31, 2020 06:21
@shazron shazron changed the title WIP feat: add update and clear commands feat: add update and clear commands Mar 31, 2020
@shazron shazron changed the title feat: add update and clear commands feat: add update and clear commands, and cli new version update core plugin Mar 31, 2020
@shazron shazron changed the title feat: add update and clear commands, and cli new version update core plugin feat: add update and rollback commands, and cli new version update core plugin Apr 1, 2020
Copy link
Member

@purplecabbage purplecabbage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, I have noted a few possible issues.
I am not crazy about us adding more commands without a topic, the aio clear command to me is a bit unclear what it does without digging deeper. ( no pun intended )

*/
async run () {
const { flags } = this.parse(UpdateCommand)
const spinner = ora()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use cli-ux.action.start/stop and not add a dependency on ora?

Copy link
Member Author

@shazron shazron Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually cli-ux is not even a dependency of the base cli (check package.json). It just so happens to be a dependency of a dependency (I'm thinking runtime plugin). Also, I needed a spinner that "goes away" after use, cli-ux's spinner does not do that, nor can I specify an empty string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the discover command uses cli-ux as well, which is not a dependency as I mentioned. In any case, we need to add cli-ux as a dependency here because of cli.table.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did it ever work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mentioned above "dependency of a dependency had cli-ux". so it was in node_modules

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to mention (See the Slack thread), aio clear is now aio rollback. I reflected it in the issue description and title when I finalized the PR yesterday.

* @param {Array<InstalledPlugin>} installedPlugins
* @returns {Array<ToUpdatePlugin}
*/
async __processPlugins () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: At some point some of us started putting double __methodPrefix to signify that something is private or not exported. Personally, I don't really like this practice.
https://github.com/airbnb/javascript#naming--leading-underscore

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I wanted the functions into a the helper module. But since these functions heavily use base command functionality (this.config, etc), I kept it here. Until we have private functions in EcmaScript classes, this visual convention would have to do. The only public contract we have with devs is the run() function, nothing else.

const { type, name, version: currentVersion } = plugin
const latestVersion = await getNpmLatestVersion(name)
let coreVersion = (type === 'core') ? currentVersion : null
const needsUpdate = (latestVersion > currentVersion)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably need to go full semver here and not just text compare, there are numerous edge cases which I don't see covered by tests. ie. v1.0.0 or 2.8.3-lts
https://www.npmjs.com/package/semver

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, i'll fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@purplecabbage PR updated

@shazron shazron merged commit 20bba9f into adobe:master Apr 2, 2020
@shazron shazron deleted the autoupdate branch April 2, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants