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
Add Torq Connector #149405
Add Torq Connector #149405
Conversation
Add Torq Connector
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.
I think we need to change the renderParameterTemplate bit to use json
and not none
, but not completely sure. Changing the tests to use an invalid character in a variable should indicate whether it's needed or not.
Other than, LGTM.
): ActionParamsType { | ||
if (!params.body) return params; | ||
return { | ||
body: renderMustacheString(params.body, variables, 'none'), |
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.
hmmm ... I think this needs to be 'json'
and not 'none'
. For example, if the character "
is embedded in one of the variable strings, and interpolated via mustache into a property value, the "
won't be escaped and the JSON won't be valid. Using 'json'
will escape characters that need to be escaped for JSON.
It would actually be good to create a test with a variable whose value contains a "
, to ensure the result is valid JSON. I think it can go in x-pack/plugins/stack_connectors/server/connector_types/torq/index.test.ts, probably just add another variable in that test ...
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.
I think I found a proper solution for this by using the renderMustacheObject
method. Also adjusted the test as you suggested to confirm we get the best of both worlds: objects are rendered without JSON escaping and strings are JSON-escaped. LMKWYT
}; | ||
if (!isUrl(value)) return error; | ||
const hostname = new URL(value).hostname; | ||
return hostname === 'hooks.torq.io' ? undefined : error; |
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.
As a future thought, since the UX doesn't seem to allow ANYTHING but the torq URL (for example, I can't use localhost
here, but it appears the server API supports it for testing), I wonder if we should at least pre-fill the value - or perhaps we don't even need it at all.
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.
This is a good point. When creating an Elastic integration on Torq, the user can copy the full webhook URL and paste it on Kibana, so in most cases a pre-filled value will be overwritten. Do you think we should pre-fill the prefix anyway, to give a hint as for what the URL should look like? My only concern is that the pre-filed value will make the validator pass even if the user fails to update it with their actual URL.
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.
Let's leave it blank then. We can polish it later, if required.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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, thanks for the param rendering change and test!
@elasticmachine merge upstream |
Thank you @pmuellr! What are the next steps for merging? |
buildkite test this |
@elasticmachine, run elasticsearch-ci/docs |
@@ -15,4 +15,6 @@ include::action-types/tines.asciidoc[leveloffset=+1] | |||
include::action-types/webhook.asciidoc[] | |||
include::action-types/cases-webhook.asciidoc[leveloffset=+1] | |||
include::action-types/xmatters.asciidoc[] | |||
include::action-types/torq.asciidoc[] | |||
include::action-types/tines.asciidoc[] |
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.
Hi @natexcvi the elasticsearch-ci/docs
check is failing because the Tines docs are already included, we are including them twice, the duplicity may have been caused by some merge.
Can you remove this line 19
? Thanks
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.
@semd just removed it
@elasticmachine, run elasticsearch-ci/docs |
buildkite test this |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Done! thanks for the contribution @natexcvi |
## Summary Add a new action type for Torq which triggers Torq workflows. This is a re-do of elastic#139635 ... ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: orihoogi <ohoogi@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@semd @MadameSheema can y'all please meet to discuss a test plan for Torq? |
## Summary Add a new action type for Torq which triggers Torq workflows. This is a re-do of elastic#139635 ... ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: orihoogi <ohoogi@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Add a new action type for Torq which triggers Torq workflows.
This is a re-do of #139635 ...
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers