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

fix: prevent postrun hook from running on uninstalled plugins #805

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

mdonnalley
Copy link
Contributor

Fixes #804

@W-14226831@

@mdonnalley mdonnalley force-pushed the mdonnalley/no-post-run-for-uninstalled branch from 3344e24 to a63f44b Compare October 3, 2023 18:08
@mdonnalley mdonnalley force-pushed the mdonnalley/no-post-run-for-uninstalled branch from a63f44b to 3f2ff19 Compare October 3, 2023 18:10
Base automatically changed from prerelease/v3 to main October 4, 2023 20:27
@mdonnalley mdonnalley force-pushed the mdonnalley/no-post-run-for-uninstalled branch from b7a9cd8 to d5f3fbe Compare October 4, 2023 21:59
// from this.plugins so that the postrun hook doesn't attempt to run any
// hooks that might have existed in the uninstalled plugins.
if (c.id === 'plugins:uninstall') {
for (const arg of argv) this.plugins.delete(arg)
Copy link
Member

Choose a reason for hiding this comment

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

plugins uninstall only accepts 1 arg but I'm guessing argv could also include --verbose. this.plugins is a map and delete doesn't throw if an element doesn't exist so this seems safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but I think plugins uninstall is supposed to take multiple arguments: https://github.com/oclif/plugin-plugins/blob/main/src/commands/plugins/uninstall.ts#L16

I'm assuming at some point variableArgs = true was changed to strict = false but the command was never updated. Iterating over the args now future proofs us from that being fixed eventually

@cristiand391
Copy link
Member

cristiand391 commented Oct 5, 2023

QA notes:

repro'd with hardis plugin on latest sf RC.
test setup:
sf repo with this branch of core linked into it, all deduped to ensure only this branch is used and a linked plugin with a postrun hook that logs to stdout:

import {Hook} from '@oclif/core'

const hook: Hook<'postrun'> = async function (opts) {
  process.stdout.write(`argv: ${opts.argv}`)
}

export default hook

./bin/run plugins uninstall sfdx-hardis uninstalls the plugin and doesn't try to fire postrun hook from hardis plugin, runs postrun hook from linked plugin
./bin/run plugins uninstall sfdx-hardis --verbose same as previous test with a bunch of output from yarn, output from test hook:

...
Uninstalling sfdx-hardis... ⡿
success Uninstalled packages.
Uninstalling sfdx-hardis... done
argv: sfdx-hardis,--verbose%

✅ unlink plugin with postrun hook, no output to stdout:

➜  sf-cli git:(main) ./bin/run plugins uninstall @cristiand391/sf-plugin-api
Uninstalling @cristiand391/sf-plugin-api... done

note: found another bug with the hook generator, see: oclif/oclif#1196

@cristiand391 cristiand391 merged commit 0c47a5c into main Oct 5, 2023
35 checks passed
@cristiand391 cristiand391 deleted the mdonnalley/no-post-run-for-uninstalled branch October 5, 2023 15:09
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.

postrun hooks of uninstalled plugins are executed
2 participants