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: use npm instead of http #154

Merged
merged 16 commits into from
Sep 7, 2021
Merged

fix: use npm instead of http #154

merged 16 commits into from
Sep 7, 2021

Conversation

maggiben
Copy link
Contributor

What does this PR do?

Replaces http commands in favor of npm commands

What issues does this PR fix or reference?

@W-9642544@

@maggiben maggiben requested a review from peternhale August 18, 2021 21:19
Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

Nice work getting all this cleaned up. Plugin-trust is super critical because it'll completely block plugin instalation if it's not working OK.

I'd like to see a NUT for this that

  1. installs any known, good signed plugin (ex: plugin-user)
  2. verifies it installs without the error message
  3. puts an unsigned plugin on the allowlist
  4. installs some known good unsigned plugin (see list https://github.com/mshanemc/awesome-sfdx-plugins. I'd suggest Jochen's jayree plugin)
  5. installs some unsigned plugin not on the allow list to verify the warning is happening

I'm not sure if there's a good NUT way to verify that this change solves private npm registry, but the NUT I'm describing would verify that the normal public repository functionality isn't impacted by these changes.

src/lib/installationVerification.ts Outdated Show resolved Hide resolved
src/lib/installationVerification.ts Outdated Show resolved Hide resolved
src/lib/installationVerification.ts Outdated Show resolved Hide resolved
src/lib/installationVerification.ts Outdated Show resolved Hide resolved
src/lib/npmCommand.ts Outdated Show resolved Hide resolved
@maggiben
Copy link
Contributor Author

maggiben commented Aug 31, 2021

Nice work getting all this cleaned up. Plugin-trust is super critical because it'll completely block plugin instalation if it's not working OK.

I'd like to see a NUT for this that

  1. installs any known, good signed plugin (ex: plugin-user)
  2. verifies it installs without the error message
  3. puts an unsigned plugin on the allowlist
  4. installs some known good unsigned plugin (see list https://github.com/mshanemc/awesome-sfdx-plugins. I'd suggest Jochen's jayree plugin)
  5. installs some unsigned plugin not on the allow list to verify the warning is happening

I'm not sure if there's a good NUT way to verify that this change solves private npm registry, but the NUT I'm describing would verify that the normal public repository functionality isn't impacted by these changes.

Hi @mshanemc I added the NUTs you suggested, luckily sfdx-trust and plugin-trust don't interfere with each other. because at this point (in the test suite) we're going to have both installed at the same time.
One of the things that Im concerned about it's not being able to test the prompt for unsigned plugins, as we discussed in the thread with @mdonnalley optional answers parameter to the execCmd function only works on Linux

.circleci/config.yml Outdated Show resolved Hide resolved
src/lib/npmCommand.ts Outdated Show resolved Hide resolved
src/lib/installationVerification.ts Show resolved Hide resolved
@mshanemc
Copy link
Contributor

mshanemc commented Sep 7, 2021

QA: can verify existing plugins are signed
published a private plugin at @salesforce/private-plugin-user which can be found if I'm auth'd on npm (but verifies as not-signed) and 404's when I logout of npm

@mshanemc mshanemc merged commit 2bce1f9 into main Sep 7, 2021
@mshanemc mshanemc deleted the bm/W-9642544 branch September 7, 2021 14:14
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.

3 participants