Add error message to resource importer result#451
Conversation
gemammercado
force-pushed
the
resourceImporter
branch
from
aca36ba to
08191c4
Compare
|
Is the goal here to allow some imports to fail + adding why the import failed? |
The goal is to add the reason why the import failed to the message the lsp returns to the client. |
gemammercado
force-pushed
the
resourceImporter
branch
from
deceda7 to
41d1f66
Compare
satyakigh
left a comment
satyakigh
left a comment
There was a problem hiding this comment.
No tests for the new failure reason attributes
gemammercado
force-pushed
the
resourceImporter
branch
from
d34cdeb to
d64904d
Compare
satyakigh
force-pushed
the
resourceImporter
branch
from
d64904d to
5970147
Compare
gemammercado
force-pushed
the
resourceImporter
branch
2 times, most recently
from
a8a4b4d to
322bc22
Compare
| @@ -75,11 +75,8 @@ export class ResourceStateManager implements SettingsConfigurable, Closeable { | |||
| } catch (error) { | |||
| if (error instanceof ResourceNotFoundException) { | |||
| log.info(`No resource found for type ${typeName} and identifier "${identifier}"`); | |||
There was a problem hiding this comment.
we can return nothing here because ResourceNotFound message is rendered client side
There was a problem hiding this comment.
That is not related to the ResourceNotFoundException here. That's just showing a client message when nothing is found which could be caused by anything on the server side. This is specifically a not found exception from CCAPI
gemammercado
force-pushed
the
resourceImporter
branch
from
2157223 to
ff13c0b
Compare
gemammercado
force-pushed
the
resourceImporter
branch
from
ff13c0b to
e61a854
Compare
|
|
||
| @Measure({ name: 'getResource', captureErrorType: true }) | ||
| public async getResource(typeName: ResourceType, identifier: ResourceId): Promise<ResourceState | undefined> { | ||
| public async getResource(typeName: ResourceType, identifier: ResourceId): Promise<GetResourceResult> { |
There was a problem hiding this comment.
Are any of the caller behaviors changed that rely on undefined?
There was a problem hiding this comment.
Yep, all callers were updated. They are:
-
ResourceStateImporter.tsbefore checkedif (resourceState)-> Now checksif(!result.resource). -
ResourceStateCompletionProvider.ts -- before checked
if (!properties)afterresourceState?.properties->if (!result.resource)and accessesresult.resource.properties -
ResourceStateManager.ts(internal searchResourceByIdentifier) -- before caught all errors to determine "not found" ->if (!result.resource)for returned errors, and still catches thrown server errors.
No caller relies on undefined anymore, they all use the { resource?, error? } shape.
| return; | ||
| } | ||
| if (isClientError(error)) { | ||
| return {}; |
There was a problem hiding this comment.
return {} is defeating the purpose of the newly added error field in GetResourceResult type.
This branch occurs when a certain identifier actually does not exist in the aws account. That kind of information is exactly what would be meaningful to as a client message.
4 participants
This PR adds an error message to the resource importer result to provide more context about why the import failed.
Client side change: aws/aws-toolkit-vscode#8632