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

[Alerting] Renamed Alerting framework AlertsClient to RulesClient according to the new terminology. #106382

Merged
merged 7 commits into from
Jul 22, 2021
Merged

[Alerting] Renamed Alerting framework AlertsClient to RulesClient according to the new terminology. #106382

merged 7 commits into from
Jul 22, 2021

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Jul 21, 2021 •
edited
Loading

Summary

Current PR renames the AlertsClient to RulesClient according to the new terminology. Resolves #105300

Changes in the alerting plugin:

  • alerts_client.ts -> rules_client.ts
  • alerts_client.mock.ts -> rules_client.mock.ts
  • alerts_client_factory.ts -> rules_client_factory.ts
  • alerts_client_factory.test.ts -> rules_client_factory.test.ts
  • alerts_client_conflict_retries.test.ts -> rules_client_conflict_retries.test.ts
  • AlertsClient -> RulesClient
  • AlertsClientFactory -> RulesClientFactory
  • alertsClientMock -> rulesClientMock
  • variables with alertsClient* wording -> rulesClient*
  • did the proper renaming changes in uptime, security_solution, monitoring and ml plugins

@YulNaumenko YulNaumenko added v8.0.0 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.15.0 Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework labels Jul 21, 2021
@YulNaumenko YulNaumenko self-assigned this Jul 21, 2021
@YulNaumenko YulNaumenko requested review from a team as code owners July 21, 2021 10:52
@YulNaumenko YulNaumenko requested a review from a team July 21, 2021 10:52
@YulNaumenko YulNaumenko requested a review from a team as a code owner July 21, 2021 10:52
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jul 21, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

dominiqueclarke
dominiqueclarke approved these changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

Uptime changes look good to me.

FrankHassanabad
FrankHassanabad approved these changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM,

What I did:

  • I read the summary which was very informative about what was being renamed
  • I briefly looked at the code to see that everything looks like a terminology renaming of code.

ymao1
ymao1 reviewed Jul 21, 2021
View reviewed changes
x-pack/plugins/alerting/server/rules_client/rules_client.ts
@@ -191,7 +191,7 @@ const alertingAuthorizationFilterOpts: AlertingAuthorizationFilterOpts = {
type: AlertingAuthorizationFilterType.KQL,
fieldNames: { ruleTypeId: 'alert.attributes.alertTypeId', consumer: 'alert.attributes.consumer' },
};
export class AlertsClient {
export class RulesClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about renaming the variables in this file as part of this terminology renaming? example alertTypeRegistry -> ruleTypeRegistry?

Copy link
Member

Choose a reason for hiding this comment

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

I noticed that as well - and I'd guess there are probably more lurking "alerts" changes to be made in the code.

Making that a follow-up issue would be fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

A follow up issue sounds good :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a good idea. Probably the changed files won't increase much :-)

ymao1
ymao1 approved these changes Jul 21, 2021
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 on green CI!

pmuellr
pmuellr approved these changes Jul 21, 2021
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM!

dasansol92
dasansol92 approved these changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@dasansol92 dasansol92 left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @YulNaumenko

@YulNaumenko YulNaumenko merged commit 4c7037b into elastic:master Jul 22, 2021
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jul 22, 2021
@YulNaumenko
[Alerting] Renamed Alerting framework AlertsClient to RulesClient acc…
cb81f98 
…ording to the new terminology. (elastic#106382)

* [Alerting] Renamed Alerting framework AlertsClient to RulesClient according to the new terminology.

* fixed path

* fixed type checks

* fixed type checks
This was referenced Jul 22, 2021
Merged
Merged
YulNaumenko added a commit that referenced this pull request Jul 22, 2021
@YulNaumenko
[Alerting] Renamed Alerting framework AlertsClient to RulesClient acc…
bd5032e 
…ording to the new terminology. (#106382) (#106508)

* [Alerting] Renamed Alerting framework AlertsClient to RulesClient according to the new terminology.

* fixed path

* fixed type checks

* fixed type checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dasansol92 dasansol92 dasansol92 approved these changes

@ymao1 ymao1 ymao1 approved these changes

@dominiqueclarke dominiqueclarke dominiqueclarke approved these changes

@darnautov darnautov darnautov approved these changes

@neptunian neptunian neptunian approved these changes

@FrankHassanabad FrankHassanabad FrankHassanabad approved these changes

@pmuellr pmuellr pmuellr approved these changes

@joeypoon joeypoon Awaiting requested review from joeypoon

Assignees

@YulNaumenko YulNaumenko

Labels
Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.15.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Alerting plugin "alerts client" terminology is confusing
10 participants
@YulNaumenko @elasticmachine @kibanamachine @pmuellr @FrankHassanabad @neptunian @darnautov @dominiqueclarke @ymao1 @dasansol92