Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Response Ops][Alerting] Adding group by options to ES query rule type #144689
[Response Ops][Alerting] Adding group by options to ES query rule type #144689
Changes from 41 commits
fc99107
57f2b79
0611e2b
a024df4
c6f04fa
db1f204
8f1830b
2614d77
682f1af
6fb8b2f
c397547
98796e1
d8b203d
a4be551
810f547
cbc920a
91a9cdd
12c222f
5dd0f57
fb37c95
1077aee
ce94cab
89f7931
6a35ffa
d498d34
0de6582
de2d9a5
97ce602
7b2be56
6312d69
c25e9a6
872d5da
194cf29
1ad9861
db5b286
d93614e
7d343ee
9d3ced8
8b527d8
ebfe25f
546dd7e
1ea9b0b
6b9bf85
e2a54c9
c4f453d
f8aadf1
c2f7e3c
ee9f5d2
4f10144
429d36c
a340635
a6e35fe
6b816b1
ab851c8
6893e1f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Did we consider defaulting these fields, if not set in the params, to the values being set here? I can see in various places in the code where we are apply defaults for these, at least in the UX. I guess when referenced by the executorIt avoids migration, but ... kinda messy/sloppy?
It's certainly nice to have it very explicit, and don't have an issue with this migration - but am kinda wondering now with the "migration changes for rolling upgrades", if this pushes us in one direction or the other ...
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 did consider it but in the past we've always taken the approach of "explicit is better" so I went for the migration instead. Definitely something we'll have to consider harder with rolling upgrades though.
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.
not sure why this
match_all
was in here, so removed.