[Agent Builder] attachments by ref#248937
Conversation
ppisljar
added
release_note:skip
leemthompo
added
the
feature:agent-builder
ppisljar
force-pushed
the
agentbuilder/attachments_by_ref
branch
5 times, most recently
from
5995444 to
6018eb4
Compare
ppisljar
force-pushed
the
agentbuilder/attachments_by_ref
branch
from
6018eb4 to
7b9d891
Compare
pgayvallet
left a comment
pgayvallet
left a comment
There was a problem hiding this comment.
Pre-approved, but added two questions
| export type VisualizationRefAttachment = Attachment< | ||
| AttachmentType.visualizationRef, | ||
| VisualizationRefAttachmentData | ||
| >; |
There was a problem hiding this comment.
NIT: You shouldn't need that second generic parameter, given you added an entry in AttachmentDataMap (see AttachmentDataOf)
| /** | ||
| * Saved objects client scoped to the current user. | ||
| * | ||
| * Optional for now to avoid forcing all runner implementations to provide it, | ||
| * but recommended for tools that need to access saved objects (e.g. by-reference attachments). | ||
| */ | ||
| savedObjectsClient?: SavedObjectsClientContract; |
There was a problem hiding this comment.
Why do we have to make that optional exactly? From the diff it's unclear to me, and I would really prefer being consistent if possible, this is fairly "dangerous" / error prone for devs.
| let resolved: unknown; | ||
| const definition = getTypeDefinition?.(attachment.type); | ||
| if (definition?.resolve && context) { | ||
| resolved = await definition.resolve( | ||
| { | ||
| id: attachmentId, | ||
| type: attachment.type, | ||
| data: versionData.data, | ||
| }, | ||
| { | ||
| request: context.request, | ||
| spaceId: context.spaceId, | ||
| savedObjectsClient: context.savedObjectsClient, | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is there a way to move that logic one layer down, in the attachmentManager ( maybe a resolve function in addition to the existing get). I assume it means moving a bunch of stuff in it (we need the request and the SO client/contract and maybe other stuff), but it would feel better in terms of isolation of concerns.
If that's not possible, I would try to at least isolate/extract that "resolving" logic in an helper function.
Asking because in the scope of #250043, I'm going to have to "replicate" the behavior from attachment tools in the FS store, and because an entry in the store should have a single "representation", so I will need to retrieve and store that "representation" for each attachment so that it's the file content in the store.
There was a problem hiding this comment.
i tried to put this inside attachmentManager. i also renamed some methods and changed the interface a bit, hopefully making it less likely for consumers to mess this up (forget resolving or sth).
| const formatVisualizationRefAttachment = (data: VisualizationRefAttachmentData): string => { | ||
| const parts: string[] = []; | ||
|
|
||
| parts.push(`[Visualization Reference]`); | ||
| parts.push(`Type: lens`); | ||
| parts.push(`ID: ${data.saved_object_id}`); | ||
|
|
||
| if (data.title) { | ||
| parts.push(`Title: ${data.title}`); | ||
| } | ||
|
|
||
| if (data.description) { | ||
| parts.push(`Description: ${data.description}`); | ||
| } | ||
|
|
||
| parts.push(`\nNote: resolve this reference using attachment_read.`); | ||
|
|
||
| return parts.join('\n'); | ||
| }; |
There was a problem hiding this comment.
NIT: following our discussion today, do we want that, or shall we just return the JSON of the ref attachment instead?
There was a problem hiding this comment.
replacing this with format() getRepresentation()
… into agentbuilder/attachments_by_ref
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
|
6 participants
Summary
resolves https://github.com/elastic/search-team/issues/12135