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: add doctor diagnostic tests for v2 crypto #939

Merged
merged 10 commits into from
Apr 5, 2024

Conversation

shetzel
Copy link
Contributor

@shetzel shetzel commented Feb 13, 2024

What does this PR do?

REQUIRES: forcedotcom/sfdx-core#810

Adds doctor diagnostic tests for v2 crypto.

What issues does this PR fix or reference?

@W-14999859@

@shetzel shetzel marked this pull request as ready for review March 4, 2024 17:26
@shetzel shetzel requested a review from a team as a code owner March 4, 2024 17:26
messages/diagnostics.md Outdated Show resolved Hide resolved
messages/diagnostics.md Outdated Show resolved Hide resolved
messages/diagnostics.md Outdated Show resolved Hide resolved
shetzel and others added 3 commits March 4, 2024 12:17
Co-authored-by: Juliet Shackell <63259011+jshackell-sfdc@users.noreply.github.com>
@cristiand391
Copy link
Member

QA notes:

with sf v2.33.3

v2 crypto + generic unix keychain

CLI supports v2 warn, CLI using stable v2 crypto fails
two 3rd party plugins use old sfdx-core, suggestions mention to go back to v1 crypto

✅ after uninstalling all 3rd party plugins, both v2 crypto diagnostics pass

✅ diagnostics also detect linked plugins with old sfdx-core

v1 crypto + generic unix keychain

✅ no plugins, diagnostics detect full v2 crypto support and that v1 is being used

v1 crypto + macos keychain

✅ detect v2 crypto supported, can't tell which one is being used.

CLI installed via installer, disabled installed node/nvm (nvm unload)

install a 3rd party plugin so that the diagnostic tries to run npm, also make sure npm is not available globally (which npm)

❌ diagnostic fails with npm: command not found:
Screenshot 2024-03-19 at 16 44 16

we can't rely on npm being installed, plugin-trust/plugins bundle npm then use that to run commands:

plugin-trust:
https://github.com/salesforcecli/plugin-trust/blob/fd3540ff98d4749c8d06a614114351b4e176a478/src/shared/npmCommand.ts#L82

plugin-plugins:
https://github.com/oclif/plugin-plugins/blob/5336f76963892f2e6dc7f4a3d7fa5787dcd02229/src/util.ts#L93

if it's not worth maybe we can disable checks that need to run npm if it's not available.

Co-authored-by: Juliet Shackell <63259011+jshackell-sfdc@users.noreply.github.com>
@cristiand391
Copy link
Member

QA update:

✅ can see auth crypto diagnostics when npm is available on PATH
✅ if no npm, test prints:

  * Couldn't determine latest CLI version due to:
/bin/sh: npm: command not found

with exit code 0

@cristiand391 cristiand391 merged commit 961fdac into main Apr 5, 2024
10 checks passed
@cristiand391 cristiand391 deleted the sh/add-doctor-diagnostics branch April 5, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants