fix: report errors when analyzing extensions #4380
Conversation
deboer-tim
requested review from
dgolovin and
lstocchi
and removed request for
a team
| return undefined; | ||
| error = `Ignoring extension ${extensionPath} without package.json file`; | ||
| console.warn(error); | ||
| return { error } as AnalyzedExtension; |
There was a problem hiding this comment.
here I think it may lead to client's error as the object they'll get is not compliant to the typescript contract.
So while I see no problem of using that pattern in tests, we should not in main code as it may lead to runtime errors.
There was a problem hiding this comment.
Understood, and I agree that would be better. I've changed it to return a regular AnalyzedExtension now. I didn't want to change the API contract, so added a couple 'fake' values like id and name (felt like these could cause problems if empty), and tried to give reasonable defaults/undefined elsewhere. I wasn't sure if there's a simpler way to do the subscription/dispose(), please review again.
Adds an error property to analyzed extensions, and analyzeExtension() sets it whenever there is a problem loading a folder instead of returning undefined. Updated callers to check for error instead of undefined, and send an error when trying to install an extension that has an error. Tests updated to confirm an error is being created. Signed-off-by: Tim deBoer <git@tdeboer.ca>
Return an AnalyzedExtension even when the manifest can't be found, with some default values. Include the required manifest properties in error message. Signed-off-by: Tim deBoer <git@tdeboer.ca>
deboer-tim
force-pushed
the
extension2
branch
from
32f81c0
to
653d588
Compare
Simplify subscriptions/dispose for error case. Signed-off-by: Tim deBoer <git@tdeboer.ca>
|
@benoitf could you take another look? |
The `as AnalyzedExtension` isn't needed anymore, since analyzeExtension() can no longer return undefined. Signed-off-by: Tim deBoer <git@tdeboer.ca>
|
@dgolovin good to remove your request (fixed) and merge? |
What does this PR do?
Since PR #2990 is very old I'm creating a new one.
Adds an error property to analyzed extensions, and analyzeExtension() sets it whenever there is a problem loading a folder instead of returning undefined. Added error for missing manifest entries.
Updated callers to check for error instead of undefined, and send an error when trying to install an extension that has an error.
Tests updated to confirm an error is being created.
Screenshot/screencast of this PR
What issues does this PR fix or reference?
Fixes #2871.
How to test this PR?
See issue #2871 - try installing this extension: quay.io/lstocchi/sample-podman-extension:v1