-
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
[RAM][Maintenance Window] Maintenance window scoped query frontend changes #171949
[RAM][Maintenance Window] Maintenance window scoped query frontend changes #171949
Conversation
@elasticmachine merge upstream |
Pinging @elastic/response-ops (Team:ResponseOps) |
@elasticmachine merge upstream |
@@ -0,0 +1,3 @@ | |||
# @kbn/alerting-types |
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.
do we need to add some explanation for future user?
const filteredIndex = data ? getFilteredIndex(data, filteredRuleTypes) : new Map(); | ||
|
||
const hasAnyAuthorizedRuleType = filteredIndex.size > 0; | ||
const authorizedRuleTypes = [...filteredIndex.values()]; | ||
const authorizedToCreateAnyRules = authorizedRuleTypes.some( | ||
(ruleType) => ruleType.authorizedConsumers[ALERTS_FEATURE_ID]?.all | ||
); | ||
const authorizedToReadAnyRules = | ||
authorizedToCreateAnyRules || | ||
authorizedRuleTypes.some((ruleType) => ruleType.authorizedConsumers[ALERTS_FEATURE_ID]?.read); | ||
|
||
return { | ||
ruleTypesState: { | ||
initialLoad: isLoading || isInitialLoading, | ||
isLoading: isLoading || isFetching, | ||
data: filteredIndex, | ||
}, | ||
hasAnyAuthorizedRuleType, | ||
authorizedRuleTypes, | ||
authorizedToReadAnyRules, | ||
authorizedToCreateAnyRules, | ||
isSuccess, | ||
}; | ||
}; |
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.
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'm not going to make this change in this PR because #171417 is also touching this hook and now we have 2 copies of it and it'll be a big mess.
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.
Ok but know we know how to do it.
); | ||
|
||
const modal = useMemo(() => { | ||
let m; |
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 thought that jsx, we will not like to render an undefined value but it will be ok with null
.
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.
react is too smart
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.
maybe it just got smarter!
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.
To test this PR, I used #172117.
The different scenario that I created to test this PR are the follows:
- I created 4 different window maintenances under main with different categories and all of them, then upgraded to your PR. I was able to see my window maintenances set up as previously.
- I created two log threshold rules with different tags and create a window maintenance with scope query around one of my tag. I was able to confirm that we still send the recover notification when the alert was active outside of the window maintenance but after that we kept this rule muted. I was also able to confirm that the other rule in the same category was still notifying me.
- I tested that when we toggle the scope query then we keep in memory the query but we do not keep the history of our category. I thing we should fix that.
- In future PR, we should make sure to validate all the fields that we use in the query by querying the fields caps API to make sure that these fields belong to index associated to our category. That will help us to be closer to the versioning API requirements and also making sure user do not use something that we will never support.
@JiaweiWu, I think we should fix #3 and I will be all good to give the green light on this awesome feature. Well done!
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as abuse.
This comment was marked as abuse.
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Partially resolves: #164255, this is 2/3 of the scoped query changes.
Maintenance window scoped query frontend changes. Adds the ability to add and edit scoped query for maintenance windows. Due to limitations with the alerts search bar and each solution fetches AAD fields, we only allow users to associate scoped query with 1 category (manangement, o11y, or security solution). The intended usage in this case is for the user to create multiple maintenance windows if they wish to apply scoped queries to multiple solutions.
To test:
go to
x-pack/plugins/alerting/public/pages/maintenance_windows/constants.ts
and setIS_SCOPED_QUERY_ENABLED
totrue
Scoped query off, multiple category allowed:
Scoped query on, multiple category disallowed:
Checklist