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

[Stack monitoring] remove support for monitoring.cluster_alerts.allowedSpaces #123229

Merged
merged 6 commits into from Jan 25, 2022

Conversation

klacabane
Copy link
Contributor

@klacabane klacabane commented Jan 18, 2022

Summary

Closes #120261

Remove support for monitoring.cluster_alerts.allowedSpaces configuration setting which is used during SM alert creation.

Testing

  • verified alert creation is still working
  • verified kibana starts with the deprecated setting configured
    [2022-01-24T13:54:14.101+01:00][WARN ][config.deprecation] You no longer need to configure "monitoring.cluster_alerts.allowedSpaces".

@klacabane klacabane self-assigned this Jan 18, 2022
@klacabane klacabane added backport-v8.0.0 release_note:deprecation Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.0.0 v8.1.0 labels Jan 18, 2022
@klacabane klacabane marked this pull request as ready for review January 18, 2022 16:02
@klacabane klacabane requested a review from a team as a code owner January 18, 2022 16:02
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@klacabane klacabane changed the title remove allowed space setting [Stack monitoring] remove support for monitoring.cluster_alerts.allowedSpaces Jan 18, 2022
Copy link
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

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

Generally LGTM, commented for leadership weigh-in. I see auto-merge is off, so I'll ✅ incase it's useful for merge later.

@@ -149,7 +149,7 @@
"signature": [
"{ ui: { elasticsearch: ",
"MonitoringElasticsearchConfig",
"; enabled: boolean; container: Readonly<{} & { logstash: Readonly<{} & { enabled: boolean; }>; apm: Readonly<{} & { enabled: boolean; }>; elasticsearch: Readonly<{} & { enabled: boolean; }>; }>; logs: Readonly<{} & { index: string; }>; metricbeat: Readonly<{} & { index: string; }>; debug_mode: boolean; debug_log_path: string; ccs: Readonly<{} & { enabled: boolean; }>; max_bucket_size: number; min_interval_seconds: number; show_license_expiration: boolean; }; tests: Readonly<{} & { cloud_detector: Readonly<{} & { enabled: boolean; }>; }>; kibana: Readonly<{} & { collection: Readonly<{} & { interval: number; enabled: boolean; }>; }>; agent: Readonly<{} & { interval: string; }>; licensing: Readonly<{} & { api_polling_frequency: moment.Duration; }>; cluster_alerts: Readonly<{} & { enabled: boolean; allowedSpaces: string[]; email_notifications: Readonly<{} & { enabled: boolean; email_address: string; }>; }>; }"
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting how the field ends up in the API definition as well.

@@ -58,7 +58,6 @@ export const configSchema = schema.object({
}),
}),
cluster_alerts: schema.object({
allowedSpaces: schema.arrayOf(schema.string(), { defaultValue: ['default'] }),
Copy link
Contributor

Choose a reason for hiding this comment

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

@smith mentioned we might want to keep this as an unused/deprecated value https://github.com/elastic/kibana/blob/6693ef371f887eca639b09c4c9b15701b4ebabd4/docs/developer/architecture/core/configuration-service.asciidoc#handle-plugin-configuration-deprecations

Though I think @jasonrhodes mentioned it wasn't actively used to begin with. Maybe fine to remove it directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flagged the setting as unused which sounds like a better ux

@klacabane
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

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

Still LGTM

@klacabane
Copy link
Contributor Author

@elasticmachine merge upstream

@klacabane
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @klacabane

@klacabane klacabane merged commit 24f0425 into elastic:main Jan 25, 2022
@klacabane klacabane deleted the 120261-allowedspaces-deprecation branch January 25, 2022 14:29
@klacabane klacabane added the auto-backport Deprecated: Automatically backport this PR after it's merged label Jan 25, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.0 Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 123229

Questions ?

Please refer to the Backport tool documentation

klacabane added a commit to klacabane/kibana that referenced this pull request Jan 25, 2022
…edSpaces (elastic#123229)

* remove allowed space setting

* set allowedSpaces as unused setting

* mock unused function in deprecation tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 24f0425)

# Conflicts:
#	api_docs/monitoring.json
klacabane added a commit that referenced this pull request Jan 25, 2022
…edSpaces (#123229) (#123736)

* remove allowed space setting

* set allowedSpaces as unused setting

* mock unused function in deprecation tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 24f0425)

# Conflicts:
#	api_docs/monitoring.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged release_note:deprecation Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation path for monitoring.cluster_alerts.allowedSpaces setting
5 participants