-
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] Allow initialization of the scope form the rule list component #173795
[RAM] Allow initialization of the scope form the rule list component #173795
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
04c230b
to
c3e992a
Compare
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.
Checked locally and work as expected 👍🏻
Added two clarification questions.
@@ -67,13 +67,20 @@ export interface RuleFormConsumerSelectionProps { | |||
consumers: RuleCreationValidConsumer[]; | |||
onChange: (consumer: RuleCreationValidConsumer | null) => void; | |||
errors: IErrorObject; | |||
selectedConsumer: RuleCreationValidConsumer | null | undefined; | |||
selectedConsumer?: RuleCreationValidConsumer | null; | |||
initialSelectedConsumer?: RuleCreationValidConsumer | 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.
Why is null
also acceptable?
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.
that's the way it was setup before me so I kept it, I will ask @Zacqary if he has any use cases for it
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.
FYI -> @Zacqary
@maryam-saeidi had the idea to use null
to avoid any kind of initialization. Therefore, rule creator can have the choice to obligated their user to set this value by themself.
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.
That works. We can differentiate between null
as an explicit uninitialized empty value, and undefined
as something waiting for its initial initialization.
...s/triggers_actions_ui/public/application/sections/rule_form/rule_form_consumer_selection.tsx
Outdated
Show resolved
Hide resolved
32218b8
to
cacd11a
Compare
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
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
@@ -97,6 +98,7 @@ export function RulesTab({ setRefresh, stateRefresh }: RulesTabProps) { | |||
onSearchFilterChange={handleSearchFilterChange} | |||
onStatusFilterChange={handleStatusFilterChange} | |||
onTypeFilterChange={handleTypeFilterChange} | |||
initialSelectedConsumer={AlertConsumers.LOGS} |
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.
@XavierM When I change this to initialSelectedConsumer={AlertConsumers.INFRASTRUCTURE}
, should I see the Metrics
as the default option? For me, it still shows Logs
as the default option.
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.
You also need to change it there -> https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/pages/rules/rules.tsx#L150 because you folks are by passing our rule creation button.
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!
…lastic#173795) ## Summary FIx _> elastic#172785 ### Checklist - [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 (cherry picked from commit 833eebf)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ponent (#173795) (#173877) # Backport This will backport the following commits from `main` to `8.12`: - [[RAM] Allow initialization of the scope form the rule list component (#173795)](#173795) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Xavier Mouligneau","email":"xavier.mouligneau@elastic.co"},"sourceCommit":{"committedDate":"2023-12-21T20:20:59Z","message":"[RAM] Allow initialization of the scope form the rule list component (#173795)\n\n## Summary\r\n\r\nFIx _> https://github.com/elastic/kibana/issues/172785\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"833eebf6107b37ac1fc505b0b619587851479d70","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","impact:high","Team:ResponseOps","v8.12.0","Team:obs-ux-management","v8.13.0"],"number":173795,"url":"https://github.com/elastic/kibana/pull/173795","mergeCommit":{"message":"[RAM] Allow initialization of the scope form the rule list component (#173795)\n\n## Summary\r\n\r\nFIx _> https://github.com/elastic/kibana/issues/172785\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"833eebf6107b37ac1fc505b0b619587851479d70"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","labelRegex":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/173795","number":173795,"mergeCommit":{"message":"[RAM] Allow initialization of the scope form the rule list component (#173795)\n\n## Summary\r\n\r\nFIx _> https://github.com/elastic/kibana/issues/172785\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"833eebf6107b37ac1fc505b0b619587851479d70"}}]}] BACKPORT--> Co-authored-by: Xavier Mouligneau <xavier.mouligneau@elastic.co>
Summary
FIx _> #172785
Checklist