Skip to content
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

Make slack param validation handle empty messages #60468

Merged

Conversation

mikecote
Copy link
Contributor

Fixes #60042

In this PR, I'm making sure the slack action type executor receives a message parameter with a minimum length of 1. This will solve the issue where slack returns a 400 error message.

@mikecote mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Mar 18, 2020
@mikecote mikecote requested a review from a team as a code owner March 18, 2020 01:51
@mikecote mikecote self-assigned this Mar 18, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@mikecote
Copy link
Contributor Author

@pmuellr further steps could be taken but I would be curious if you think this would be sufficient?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - at least we'll get a failure at execution time

We may want to do similar for things like email/log subject/message if we don't already, but they seem like it might be more obvious than the way Slack seems to silently eat these.

And we may also want better checking in the client, for non-existant mustache template vars, but that seems hard right now.

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikecote
Copy link
Contributor Author

We may want to do similar for things like email/log subject/message if we don't already, but they seem like it might be more obvious than the way Slack seems to silently eat these.

And we may also want better checking in the client, for non-existant mustache template vars, but that seems hard right now.

Good point, I went ahead and opened up an issue to follow up #60614.

@mikecote mikecote merged commit 27045e0 into elastic:master Mar 19, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Mar 19, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 19, 2020
…-cluster-replication

* 'master' of github.com:elastic/kibana: (89 commits)
  Sort by name when fetching alerts and connectors (elastic#60506)
  Make slack param validation handle empty messages (elastic#60468)
  [Alerting] Adds navigation by consumer and alert type to alerting (elastic#58997)
  Introduce search interceptor (elastic#60523)
  [ML] Add functional tests for file data visualizer (elastic#60413)
  [APM] Optimize service map query (elastic#60412)
  [SIEM][Detection Engine] Adds lists feature flag and list values to the REST interfaces
  Enhancement/update esdocs datasource (elastic#59512)
  [junit] only include stdout in report for failures (elastic#60530)
  Update dependency nock to v12 (elastic#60422)
  upgrade execa to get stdout/stderr in error messages (elastic#60537)
  skip flaky suite (elastic#60471)
  [Ingest] Agent Config Details - Data sources list ui (elastic#60429)
  [SIEM] Create ML Rules (elastic#58053)
  skip flaky suite (elastic#60559)
  fix agent type (elastic#60554)
  Fixed default message for index threshold includes both threshold values (elastic#60545)
  [Ingest] Add support for `yaml` field types (elastic#60440)
  Solved the issue for a GROUP BY expression validation (elastic#60558)
  [Maps] Mark instance state as readonly (elastic#60557)
  ...

# Conflicts:
#	x-pack/legacy/plugins/cross_cluster_replication/public/np_ready/app/components/auto_follow_pattern_form.js
#	x-pack/legacy/plugins/cross_cluster_replication/public/np_ready/app/components/follower_index_form/follower_index_form.js
#	x-pack/legacy/plugins/cross_cluster_replication/public/np_ready/app/components/follower_index_form/follower_index_form.test.js
#	x-pack/legacy/plugins/cross_cluster_replication/public/np_ready/app/services/auto_follow_pattern_validators.js
#	x-pack/legacy/plugins/cross_cluster_replication/public/np_ready/app/services/input_validation.js
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 19, 2020
…ole/cleanup

* 'master' of github.com:elastic/kibana: (47 commits)
  [Remote clusters] Update copy (elastic#60382)
  Sort by name when fetching alerts and connectors (elastic#60506)
  Make slack param validation handle empty messages (elastic#60468)
  [Alerting] Adds navigation by consumer and alert type to alerting (elastic#58997)
  Introduce search interceptor (elastic#60523)
  [ML] Add functional tests for file data visualizer (elastic#60413)
  [APM] Optimize service map query (elastic#60412)
  [SIEM][Detection Engine] Adds lists feature flag and list values to the REST interfaces
  Enhancement/update esdocs datasource (elastic#59512)
  [junit] only include stdout in report for failures (elastic#60530)
  Update dependency nock to v12 (elastic#60422)
  upgrade execa to get stdout/stderr in error messages (elastic#60537)
  skip flaky suite (elastic#60471)
  [Ingest] Agent Config Details - Data sources list ui (elastic#60429)
  [SIEM] Create ML Rules (elastic#58053)
  skip flaky suite (elastic#60559)
  fix agent type (elastic#60554)
  Fixed default message for index threshold includes both threshold values (elastic#60545)
  [Ingest] Add support for `yaml` field types (elastic#60440)
  Solved the issue for a GROUP BY expression validation (elastic#60558)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 19, 2020
* master: (39 commits)
  [SIEM][CASE]  Configuration page action bar (elastic#60608)
  [Remote clusters] Update copy (elastic#60382)
  Sort by name when fetching alerts and connectors (elastic#60506)
  Make slack param validation handle empty messages (elastic#60468)
  [Alerting] Adds navigation by consumer and alert type to alerting (elastic#58997)
  Introduce search interceptor (elastic#60523)
  [ML] Add functional tests for file data visualizer (elastic#60413)
  [APM] Optimize service map query (elastic#60412)
  [SIEM][Detection Engine] Adds lists feature flag and list values to the REST interfaces
  Enhancement/update esdocs datasource (elastic#59512)
  [junit] only include stdout in report for failures (elastic#60530)
  Update dependency nock to v12 (elastic#60422)
  upgrade execa to get stdout/stderr in error messages (elastic#60537)
  skip flaky suite (elastic#60471)
  [Ingest] Agent Config Details - Data sources list ui (elastic#60429)
  [SIEM] Create ML Rules (elastic#58053)
  skip flaky suite (elastic#60559)
  fix agent type (elastic#60554)
  Fixed default message for index threshold includes both threshold values (elastic#60545)
  [Ingest] Add support for `yaml` field types (elastic#60440)
  ...
@mikecote mikecote added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting] creating alert with slack action with bad action variable results in 400 response on execution
5 participants