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

Don't load connectors and connector types when there isn't an encryptionKey #133335

Merged
merged 1 commit into from
Jun 3, 2022
Merged

Don't load connectors and connector types when there isn't an encryptionKey #133335

merged 1 commit into from
Jun 3, 2022

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Jun 1, 2022 •
edited
Loading

fixes:#131986

Http requests to connectors and connector-types endpoints return error when there isn't a permanent encryptionKey.
This PR intends to stop making those redundant http requests triggered by RuleAdd flyout.

I also found out that RuleAdd flyout was added to the Discover page twice. This PR removes one of them too.

@ersin-erdal
Remove duplicated AddRule flyout from Discover page.
7229abd 
Wait for health check result before making requests to connectors and connector types.
@ersin-erdal ersin-erdal added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework v8.3.0 labels Jun 1, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
discover 515.6KB 515.6KB -2.0B

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

@ersin-erdal ersin-erdal changed the title Remove duplicated AddRule flyout from Discover page. Don't load connectors and connector types when there isn't an encryptionKey Jun 1, 2022
ersin-erdal
ersin-erdal commented Jun 1, 2022
View reviewed changes
src/plugins/discover/public/application/main/components/top_nav/open_alerts_popover.tsx
@@ -108,12 +108,7 @@ export function AlertsPopover({
name: 'Alerting',
items: [
{
name: (
<>
{SearchThresholdAlertFlyout}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Flyout is already added on line 137.
Removing this to avoid multiple renders and redundant http requests.

Copy link
Member

Choose a reason for hiding this comment

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

thx for fixing this! 👍

@ersin-erdal ersin-erdal marked this pull request as ready for review June 2, 2022 13:40
@ersin-erdal ersin-erdal requested review from a team as code owners June 2, 2022 13:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@kertal kertal requested a review from dimaanj June 2, 2022 13:42
dimaanj
dimaanj approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@dimaanj dimaanj left a comment

Choose a reason for hiding this comment

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

The code LGTM.

ymao1
ymao1 approved these changes Jun 2, 2022
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! Verified that when running without encryption key, the flyout doesn't try to load connector types and doesn't show discard modal on close. Verified that no more duplicate API calls are made. Verified that can create search threshold rule as expected when encryption key is set. Nice job!

mikecote
mikecote approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Tested locally and LGTM!

kertal
kertal approved these changes Jun 3, 2022
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM, code review of DataDiscovery.team owned code, thx for the fix!

@cnasikas cnasikas mentioned this pull request Jun 3, 2022
Open
@ersin-erdal ersin-erdal merged commit fc76e0c into elastic:main Jun 3, 2022
@ersin-erdal ersin-erdal deleted the 131986-create-threshold-alert-no-encryption-key branch June 3, 2022 16:30
mibragimov pushed a commit to mibragimov/kibana that referenced this pull request Jun 7, 2022
@ersin-erdal
Remove duplicated AddRule flyout from Discover page. (elastic#133335)
88feca7 
Wait for health check result before making requests to connectors and connector types.
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 7, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 133335 locally

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 133335 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 133335 locally

@kertal kertal added the v8.4.0 label Jun 9, 2022
@kertal
Copy link
Member

kertal commented Jun 9, 2022

dear @ersin-erdal, can you backport ? many thx

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 133335 locally

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 133335 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 133335 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 133335 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 133335 locally

@ersin-erdal ersin-erdal mentioned this pull request Jun 17, 2022
Merged
ersin-erdal added a commit to ersin-erdal/kibana that referenced this pull request Jun 17, 2022
@ersin-erdal
Remove duplicated AddRule flyout from Discover page. (elastic#133335)
1fe8c86 
Wait for health check result before making requests to connectors and connector types.

(cherry picked from commit fc76e0c)
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

ersin-erdal added a commit that referenced this pull request Jun 17, 2022
@ersin-erdal
Remove duplicated AddRule flyout from Discover page. (#133335) (#134689)
20f6646 
Wait for health check result before making requests to connectors and connector types.

(cherry picked from commit fc76e0c)
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 17, 2022
@mikecote mikecote mentioned this pull request Oct 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

@mikecote mikecote mikecote approved these changes

@ymao1 ymao1 ymao1 approved these changes

@dimaanj dimaanj dimaanj approved these changes

Assignees
No one assigned
Labels
Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.3.0 v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ersin-erdal @kibana-ci @elasticmachine @kibanamachine @kertal @mikecote @ymao1 @dimaanj