-
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
[Logs UI] [Alerting] Alerts management page enhancements #64654
[Logs UI] [Alerting] Alerts management page enhancements #64654
Conversation
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
@elasticmachine merge upstream |
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.
Pulled the code. I couldn't find anything not working 🎉
import { FormattedMessage } from '@kbn/i18n/react'; | ||
import { | ||
ForLastExpression, | ||
// eslint-disable-next-line @kbn/eslint/no-restricted-paths | ||
} from '../../../../../../triggers_actions_ui/public/common'; | ||
// eslint-disable-next-line @kbn/eslint/no-restricted-paths | ||
import { IErrorObject } from '../../../../../../triggers_actions_ui/public/types'; | ||
import { useSource } from '../../../../containers/source'; | ||
// eslint-disable-next-line @kbn/eslint/no-restricted-paths |
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 you know why this path might be restricted? Maybe it's a bug and it shouldn't
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.
Yeah, I need to follow this up with the Alerting team. There's a few types we're (logs and metrics) using that aren't exported from public
or server
, and so aren't really supposed to be used. Once they're bumped up to being part of the public
or server
contract, I'll remove the lint overrides. I'll handle in a followup 👍
@elasticmachine merge upstream |
@afgomez Thanks for the review 👍 |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Ensure adding / editing log alerts works from the alerts management page
…ana into alerting/np-tests-migration * 'alerting/np-tests-migration' of github.com:gmmorris/kibana: [APM] Agent remote config: validation for Java agent configs (elastic#63956) [APM] Fix duplicate index patterns (elastic#64883) Drilldowns (elastic#61219) [Alerting] fix labels and links in PagerDuty action ui and docs (elastic#64032) [Event Log] Ensure sorting tests are less flaky (elastic#64781) update endpoint to restrict removing with datasources (elastic#64978) [Logs UI] [Alerting] Alerts management page enhancements (elastic#64654) Adjust kibana app owning files (elastic#65064) Migrate tutorial resources (elastic#64298) [Logs UI] Tweak copy in log alerts dialog (elastic#64645) [Logs UI] [Alerting] Documentation (elastic#64886) [Logs UI] Add dataset filter to ML module setup screen (elastic#64470) [TSVB] Fixing memory leak (elastic#64918) Bump backport to 5.4.1 (elastic#65041)
…5070) * Ensure adding / editing log alerts works from the alerts management page Co-authored-by: Elastic Machine <[email protected]>
Summary
This PR:
fetch
is an injected dependency, which also kills off severallegacy_singleton
uses.Testing
mount
, instead ofsetup
, to reduce bundle sizes which breaks editing on the alerts management page. If someone starts reviewing this before this is sorted with Ops you can simply move the linethis.registerLogsAlertType(pluginsSetup);
frommount
tosetup
Alerting requires SSL, with a local setup this can be enabled like so:
yarn es snapshot --ssl
yarn start --ssl
ssl.verification_mode: none
inside of the Filebeat config.Things to check: