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.

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

Add comments to some alerting plugin public API items #101551

Merged

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Jun 8, 2021

  1. Add comments on some of the public API items
  2. Update some functions to use the new terminology of rule instead of alert.
  3. Remove the second unused parameter to AlertNavigationHandler. No consumers were using it and it seems like it should be extractible from the rule that is passed in. Having it as a second parameter opens the door for passing in a rule type that the first parameter's rule doesn't match. What should happen then?

Before:

Screen Shot 2021-06-07 at 10 37 32 PM

After:
Screen Shot 2021-06-08 at 8 26 35 AM

@stacey-gammon stacey-gammon changed the title 2021 06 07 alert comments api cleanup Add comments to some alerting plugin public API items Jun 8, 2021
@stacey-gammon stacey-gammon added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0 labels Jun 8, 2021
@stacey-gammon stacey-gammon requested a review from mikecote June 8, 2021 13:37
@stacey-gammon stacey-gammon requested a review from pmuellr June 8, 2021 18:37
@stacey-gammon stacey-gammon marked this pull request as ready for review June 8, 2021 18:48
@stacey-gammon stacey-gammon requested a review from a team as a code owner June 8, 2021 18:48
@stacey-gammon stacey-gammon added the Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) label Jun 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@stacey-gammon
Copy link
Contributor Author

@elasticmachine merge upstream

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 with some minor wording nits!

x-pack/plugins/alerting/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/public/plugin.ts Outdated Show resolved Hide resolved
@stacey-gammon stacey-gammon enabled auto-merge (squash) June 10, 2021 17:08
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 213 206 -7

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
alerting 15 14 -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
alerting 61.4KB 61.3KB -154.0B
Unknown metric groups

API count

id before after diff
alerting 213 214 +1

History

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

@stacey-gammon stacey-gammon merged commit e9a4028 into elastic:master Jun 10, 2021
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 14, 2021
* master: (68 commits)
  skip flaky suite (elastic#94043)
  skip flaky suite (elastic#102012)
  [esArchive] Persists updates for management/saved_objects/* (elastic#101992)
  skip flaky suite (elastic#101449)
  remove unnecessary hack (elastic#101909)
  [Exploratory View] Use human readable formats (elastic#101520)
  [Expressions] Refactor expression functions to use observables underneath (elastic#100409)
  [esArchives] Persist migrated Kibana archives (elastic#101950)
  [kbnArchiver] fix save to non-existent file (elastic#101974)
  [Enterprise Search] Add owner and description properties to kibana.json (elastic#101957)
  [DOCS] Fixes terminology in Stack Monitoring:Kibana alerts (elastic#101696)
  [Observability] [Cases] Cases in the observability app (elastic#101487)
  [Alerting][Docs] Combine rule creation and management pages (elastic#101498)
  temporarily disable build-buddy
  [Fleet] Fix fleet server collector in case settings are not set (elastic#101752)
  [Event Log] Populated rule.* ECS fields for alert events. (elastic#101132)
  [APM] Fleet support for merging input.config values with other nested properties in the policy input (elastic#101690)
  Add comments to some alerting plugin public API items (elastic#101551)
  [Alerting][Docs] Moving alerting setup to its own page (elastic#101323)
  remove uptime public API, it's not used. (elastic#101799)
  ...
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 101551 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 14, 2021
@stacey-gammon stacey-gammon added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 14, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 14, 2021
* Add comments. Remove ruleType as the second param, not needed.

* Add comments. Remove ruleType as the second param, not needed.

* Fix bad type check and update docs

* update docs

* Remove unused import

* change exports to type to avoid increasing bundle size

* Update x-pack/plugins/alerting/public/plugin.ts

Co-authored-by: ymao1 <[email protected]>

* Update x-pack/plugins/alerting/public/plugin.ts

Co-authored-by: ymao1 <[email protected]>

* Update x-pack/plugins/alerting/public/plugin.ts

Co-authored-by: ymao1 <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: ymao1 <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 14, 2021
kibanamachine added a commit that referenced this pull request Jun 14, 2021
)

* Add comments. Remove ruleType as the second param, not needed.

* Add comments. Remove ruleType as the second param, not needed.

* Fix bad type check and update docs

* update docs

* Remove unused import

* change exports to type to avoid increasing bundle size

* Update x-pack/plugins/alerting/public/plugin.ts

Co-authored-by: ymao1 <[email protected]>

* Update x-pack/plugins/alerting/public/plugin.ts

Co-authored-by: ymao1 <[email protected]>

* Update x-pack/plugins/alerting/public/plugin.ts

Co-authored-by: ymao1 <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: ymao1 <[email protected]>

Co-authored-by: Stacey Gammon <[email protected]>
Co-authored-by: ymao1 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants