[Automatic Import] Prepare to support more connectors #191278
Conversation
P1llus
added
enhancement
P1llus
changed the title
|
Pinging @elastic/security-scalability (Team:Security-Scalability) |
| } | ||
|
|
||
| export interface CategorizationNodeParams extends CategorizationBaseNodeParams { | ||
| model: ChatModels; |
There was a problem hiding this comment.
Can we not do it other way? Like
export interface BaseNodeParams {
model: ChatModels;
}
export interface CategorizationNodeParams extends BaseNodeParams {
state: CategorizationState;
}And reuse BaseNodeParams in all graphs
There was a problem hiding this comment.
We could yeah, however the issue with abstraction is that you would leave it very hard to perform changes on just one graph that might be model related etc.
At that point we start making a type that extends BaseNodeParams only for that graph, and later another one might appear etc.
I don't mind moving BaseNodeParams up to /server/types.ts, and reuse it for all the graphs, but we should be careful to not push the abstractions too wide, as it always makes changes later messier.
There was a problem hiding this comment.
Yeah... Just seen that state is being used alone and not model... So no point abstracting model.
Probably we leave it this way to be consistent and not make it complex
| } | ||
|
|
||
| export interface CategorizationGraphParams { | ||
| model: ChatModels; |
There was a problem hiding this comment.
And model can be reused from BaseNodeParams
There was a problem hiding this comment.
As long as we do not want to separate graph compilation parameters we could reuse that yes, I don't have a strong opinion on this one.
| @@ -19,7 +16,7 @@ import { handleMissingKeys } from './missing'; | |||
| import { handleValidateMappings } from './validate'; | |||
| import { graphState } from './state'; | |||
|
|
|||
| const handleCreateMappingChunks = async (state: EcsMappingState) => { | |||
| const handleCreateMappingChunks = async ({ state }: EcsBaseNodeParams) => { | |||
There was a problem hiding this comment.
Do we plan to extend the Params apart from state? Else we dan directly use EcsMappingState right?
There was a problem hiding this comment.
Do we not want to keep that consistent over all the nodes for now? It is in theory using EcsMapping state right now, just with the interface name?
There was a problem hiding this comment.
yeah , we do want to have it consistent. But was thinking did we miss anything to pass in other nodes?
x-pack/plugins/integration_assistant/server/graphs/categorization/validate.ts
Show resolved
Hide resolved
| try { | ||
| response = await ecsGraph.invoke(mockedRequest); | ||
| } catch (error) { | ||
| throw Error(`getEcsGraph threw an error: ${error}`); |
There was a problem hiding this comment.
Do we really benefit from catching an error if we immediately rethrow?
There was a problem hiding this comment.
Tbh this one was a bit weird, because we had some references to the fail function rather than throwing, however jest does not recognise fail anymore, and if I ignore the catch and just //noop, then it sometimes ignores or hides the errors I am looking for (so I end up having to log the error by making a similar change each time I want to debug), so this time I just kept it in, I can remove it though if it did not make sense.
💛 Build succeeded, but was flaky
Failed CI Steps
Metrics [docs]
To update your PR or re-run it, just comment with: |
P1llus
added
backport:skip
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
P1llus
added
backport:prev-minor
This PR does not add any functionality, it adds interfaces to the expected parameters from get*Graph and its graph nodes. This is so it will be much easier extend this later when we might need to add/switch types over a whole graph like we would have needed when adding more connectors. The PR touches a lot of files, but does not add/remove/change any functionality at all, and the current expected function arguments are the same, just the format is a bit different to better align with how other plugins are doing it. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios. (cherry picked from commit 791f638)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
This PR does not add any functionality, it adds interfaces to the expected parameters from get*Graph and its graph nodes. This is so it will be much easier extend this later when we might need to add/switch types over a whole graph like we would have needed when adding more connectors. The PR touches a lot of files, but does not add/remove/change any functionality at all, and the current expected function arguments are the same, just the format is a bit different to better align with how other plugins are doing it. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios. (cherry picked from commit 791f638)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
#191525) # Backport This will backport the following commits from `main` to `8.15`: - [[Automatic Import] Prepare to support more connectors (#191278)](#191278) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Marius Iversen","email":"marius.iversen@elastic.co"},"sourceCommit":{"committedDate":"2024-08-26T17:29:30Z","message":"[Automatic Import] Prepare to support more connectors (#191278)\n\nThis PR does not add any functionality, it adds interfaces to the\nexpected parameters from get*Graph and its graph nodes.\nThis is so it will be much easier extend this later when we might need\nto add/switch types over a whole graph like we would have needed when\nadding more connectors.\n\nThe PR touches a lot of files, but does not add/remove/change any\nfunctionality at all, and the current expected function arguments are\nthe same, just the format is a bit different to better align with how\nother plugins are doing it.\n\n\n\n- [x] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios.","sha":"791f638823820e8c8cc5af31e56a4f0875356dc9","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["enhancement","release_note:skip","backport:prev-minor","v8.16.0","Team:Security-Scalability","v8.15.1"],"title":"[Automatic Import] Prepare to support more connectors","number":191278,"url":"https://github.com/elastic/kibana/pull/191278","mergeCommit":{"message":"[Automatic Import] Prepare to support more connectors (#191278)\n\nThis PR does not add any functionality, it adds interfaces to the\nexpected parameters from get*Graph and its graph nodes.\nThis is so it will be much easier extend this later when we might need\nto add/switch types over a whole graph like we would have needed when\nadding more connectors.\n\nThe PR touches a lot of files, but does not add/remove/change any\nfunctionality at all, and the current expected function arguments are\nthe same, just the format is a bit different to better align with how\nother plugins are doing it.\n\n\n\n- [x] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios.","sha":"791f638823820e8c8cc5af31e56a4f0875356dc9"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/191278","number":191278,"mergeCommit":{"message":"[Automatic Import] Prepare to support more connectors (#191278)\n\nThis PR does not add any functionality, it adds interfaces to the\nexpected parameters from get*Graph and its graph nodes.\nThis is so it will be much easier extend this later when we might need\nto add/switch types over a whole graph like we would have needed when\nadding more connectors.\n\nThe PR touches a lot of files, but does not add/remove/change any\nfunctionality at all, and the current expected function arguments are\nthe same, just the format is a bit different to better align with how\nother plugins are doing it.\n\n\n\n- [x] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios.","sha":"791f638823820e8c8cc5af31e56a4f0875356dc9"}},{"branch":"8.15","label":"v8.15.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Marius Iversen <marius.iversen@elastic.co>
This PR does not add any functionality, it adds interfaces to the expected parameters from get*Graph and its graph nodes.
This is so it will be much easier extend this later when we might need to add/switch types over a whole graph like we would have needed when adding more connectors.
The PR touches a lot of files, but does not add/remove/change any functionality at all, and the current expected function arguments are the same, just the format is a bit different to better align with how other plugins are doing it.