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.

Sign up for GitHub

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

Jump to bottom

[Actions] fixes bug where severity is auto selected but not applied to the action in PagerDuty #84891

Merged
merged 4 commits into from
Dec 4, 2020
Merged

[Actions] fixes bug where severity is auto selected but not applied to the action in PagerDuty #84891

merged 4 commits into from
Dec 4, 2020

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Dec 3, 2020 •
edited
Loading

Summary

Closes #84799

In this PR we ensure the EuiSelects in the PagerDuty params components don't auto select a value when the field doesn't have a default value.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@gmmorris gmmorris requested a review from a team as a code owner December 3, 2020 12:08
@gmmorris gmmorris added Feature:Actions Feature:Alerting release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0 labels Dec 3, 2020
@elasticmachine
Copy link
Contributor

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

ymao1
ymao1 reviewed Dec 3, 2020
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

I am able to save an Alert without selecting a PagerDuty severity. Since the severity field is not marked optional, I think this behavior should not be allowed.

@gmmorris
Copy link
Contributor Author

gmmorris commented Dec 3, 2020 •
edited
Loading

I am able to save an Alert without selecting a PagerDuty severity. Since the severity field is not marked optional, I think this behavior should not be allowed.

Good catch.... I think it's supposed to be optional... not sure 🤔

I think users can configure a default severity in PD, and so it doesn't need to be required on our end.
Looking at the connector looks like it wasn't meant to be a required field (based on validations).

@YulNaumenko any idea?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
triggersActionsUi 284 285 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 1.5MB 1.5MB +1.2KB

History

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

@YulNaumenko
Copy link
Contributor

I am able to save an Alert without selecting a PagerDuty severity. Since the severity field is not marked optional, I think this behavior should not be allowed.

Good catch.... I think it's supposed to be optional... not sure 🤔

I think users can configure a default severity in PD, and so it doesn't need to be required on our end.
Looking at the connector looks like it wasn't meant to be a required field (based on validations).

@YulNaumenko any idea?

In the PagerDuty UI experience Severity is an optional field and according to the doc Unknown
automatically chosen when a tool is not setting or can not set the severity.

YulNaumenko
YulNaumenko approved these changes Dec 3, 2020
View reviewed changes
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

@gmmorris
Copy link
Contributor Author

gmmorris commented Dec 3, 2020

I am able to save an Alert without selecting a PagerDuty severity. Since the severity field is not marked optional, I think this behavior should not be allowed.

Good catch.... I think it's supposed to be optional... not sure 🤔
I think users can configure a default severity in PD, and so it doesn't need to be required on our end.
Looking at the connector looks like it wasn't meant to be a required field (based on validations).
@YulNaumenko any idea?

In the PagerDuty UI experience Severity is an optional field and according to the doc Unknown
automatically chosen when a tool is not setting or can not set the severity.

Awesome, with that in mind I've made the field optional.

ymao1
ymao1 approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM

@gmmorris gmmorris merged commit ad49853 into elastic:master Dec 4, 2020
@gmmorris gmmorris mentioned this pull request Dec 4, 2020
Merged
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 4, 2020
@gmmorris
[Actions] fixes bug where severity is auto selected but not applied t…
7c0096c 
…o the action in PagerDuty (elastic#84891)

In this PR we ensure the EuiSelects in the PagerDuty params components don't auto select a value when the field doesn't have a default value.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 4, 2020
@gmmorris
Merge branch 'master' into alerts/custom-recovery-action-group
8940592 
* master: (28 commits)
  [Actions] fixes bug where severity is auto selected but not applied to the action in PagerDuty (elastic#84891)
  Only attempt to rollover signals index if version < builtin version (elastic#84982)
  skip flaky suite (elastic#84978)
  skip lens rollup tests
  Add geo containment tracking alert type (elastic#84151)
  Changed rollup tests to use test user rather than elastic super user. (elastic#79567)
  skip 'should allow creation of lens xy chart' elastic#84957
  [APM] Add log_level/sanitize_field_names config options to Python Agent (elastic#84810)
  [Maps] geo line source (elastic#76572)
  [data.search] Move search method inside session service and add tests (elastic#84715)
  skip lens drag and drop test.  elastic#84941
  [Ingest Node Pipelines] Integrate painless autocomplete (elastic#84554)
  [Lens] allow drag and drop reorder on xyChart for y dimension (elastic#84640)
  [Lens] Fix error when selecting the current field again (elastic#84817)
  [Metrics UI] Add metadata tab to node details flyout (elastic#84454)
  [CI] Enables APM collection (elastic#81731)
  [Workplace Search] Migrate Sources Schema tree (elastic#84847)
  Disable checking for conflicts when copying saved objects (elastic#83575)
  [SECURITY_SOLUTION] delete advanced Policy fields when they are empty (elastic#84368)
  y18n 4.0.0 -> 4.0.1 (elastic#84905)
  ...
gmmorris added a commit that referenced this pull request Dec 4, 2020
@gmmorris
[Actions] fixes bug where severity is auto selected but not applied t…
b63d0a6 
…o the action in PagerDuty (#84891) (#85000)

In this PR we ensure the EuiSelects in the PagerDuty params components don't auto select a value when the field doesn't have a default value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@ymao1 ymao1 ymao1 approved these changes

Assignees
No one assigned
Labels
Feature:Actions Feature:Alerting release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Default field in PagerDuty action isn't actually applied
5 participants
@gmmorris @elasticmachine @kibanamachine @YulNaumenko @ymao1