Fix plugin install error not displayed correctly #955
Conversation
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #955 +/- ##
==========================================
+ Coverage 89.39% 89.51% +0.11%
==========================================
Files 207 207
Lines 11264 11259 -5
Branches 2506 2507 +1
==========================================
+ Hits 10070 10078 +8
+ Misses 1194 1181 -13
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
53ea3f7
to
9abfba4
Compare
Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
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.
Changes LGTM!
Will approve once we determine if removing getNpmPath
is ok 😋
if (result.stderr?.length > 0) { | ||
msg += `\n${result.stderr.toString()}`; | ||
} | ||
throw new Error(msg); |
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.
Should we throw an ImperativeError?
There may be other places where we throw non Imperative errors. For example:
throw new Error("Failed to uninstall plugin, install folder still exists:\n " + installFolder);
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 method is intended to be low-level and mimic the error handling behavior of child_process.execSync
. If we throw an Imperative error that wraps the original error, it may affect how the original error gets logged. I'm open to switching if we think that using vanilla JS errors is bad practice here 🙂
Kudos, SonarCloud Quality Gate passed! |
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
Fixes #954 by adding utility method
ProcessUtils.execAndCheckOutput
that wrapschild_process.spawnSync
for better error handling.Notes:
NpmFunctions.ts
and removed an unused one - not a breaking change since this module is not exportedhttp://localhost:4873/
as npm registry in some tests is that's the default port used by Verdaccio 🙂