-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Tines connector] SubActions framework changes #142475
Conversation
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
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.
Thank you for the fixes!
@elasticmachine merge upstream |
x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/execute.ts
Show resolved
Hide resolved
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.
LGTM, but added a note about a missing test for the newly supported renderParameterTemplates
parameter
@@ -54,5 +54,6 @@ export const register = <Config extends ActionTypeConfig, Secrets extends Action | |||
supportedFeatureIds: connector.supportedFeatureIds, | |||
validate: validators, | |||
executor, | |||
renderParameterTemplates: connector.renderParameterTemplates, |
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.
Seems like we should have a test for this new parameter being copied in, in register.test.ts
.
@@ -55,6 +56,7 @@ describe('Registration', () => { | |||
supportedFeatureIds: connector.supportedFeatureIds, | |||
validate: expect.anything(), | |||
executor: expect.anything(), | |||
renderParameterTemplates: expect.anything(), |
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.
nit: This check does not assert that the correct function is passed. What about
const renderParameterTemplates = jest.fn()
const connector = {
id: '.test',
name: 'Test',
minimumLicenseRequired: 'basic' as const,
supportedFeatureIds: ['alerting'],
schema: {
config: TestConfigSchema,
secrets: TestSecretsSchema,
},
Service: TestSubActionConnector,
renderParameterTemplates,
};
// code omitted for brevity
expect(actionTypeRegistry.register).toHaveBeenCalledWith({
id: connector.id,
name: connector.name,
minimumLicenseRequired: connector.minimumLicenseRequired,
supportedFeatureIds: connector.supportedFeatureIds,
validate: expect.anything(),
executor: expect.anything(),
renderParameterTemplates,
});
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.
Sure, I was following the same criteria as in the validate
and executor
properties.
However, strictly speaking, it does not need to be the same function, the important thing is the function is properly wired.
Instead of testing the function reference (it could change and still be correct), I updated the test by adding a test case that checks the renderParameterTemplates
is called with the correct parameters and it returns the correct value.
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.
You are right @semd! We should improve that. Thanks for the additional test. Much better.
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.
@cnasikas 👍 thanks for the review!
Pinging @elastic/response-ops (Team:ResponseOps) |
….com/semd/kibana into 140066_sub_actions_framework_changes
…framework_changes
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: cc @semd |
* validators and template render * frontend use_sub_action changes * rollback custom validations in favor of 142376 * types fix * rollback test * extract render template function type * fix type * use abort controller in useSubAction hook * renderTemplate in register test * [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs' * improve test * register test improved * test the renderedTemrenderedTemplate return value Co-authored-by: Kibana Machine <[email protected]>
* validators and template render * frontend use_sub_action changes * rollback custom validations in favor of 142376 * types fix * rollback test * extract render template function type * fix type * use abort controller in useSubAction hook * renderTemplate in register test * [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs' * improve test * register test improved * test the renderedTemrenderedTemplate return value Co-authored-by: Kibana Machine <[email protected]>
Summary
Issue: #140066
This PR is part of the Tines connector implementation, which is being implemented using the SubActions framework.
It needed some changes in the SubAction framework in order to meet the requirements, these changes have been extracted into this PR to be reviewed independently, the connector implementation itself is not included.
The SubActions framework changes are:
Backend:
renderParameterTemplates
to allow custom body rendering.Frontend:
useSubAction
hook was improved to prevent an infinite request loop.