-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Deprecate xpack:defaultAdminEmail for monitoring alerts #22195
Changes from 8 commits
54c581f
3b0f486
c771602
2605f2f
a8b186d
5eacfe1
ca5a8a1
bbbc344
820d2c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
*/ | ||
|
||
import { get, has, set } from 'lodash'; | ||
import { CLUSTER_ALERTS_ADDRESS_CONFIG_KEY } from './common/constants'; | ||
|
||
/** | ||
* Re-writes deprecated user-defined config settings and logs warnings as a | ||
|
@@ -29,12 +30,19 @@ export const deprecations = ({ rename }) => { | |
delete settings.elasticsearch.ssl.verify; | ||
|
||
log('Config key "xpack.monitoring.elasticsearch.ssl.verify" is deprecated. ' + | ||
'It has been replaced with "xpack.monitoring.elasticsearch.ssl.verificationMode"'); | ||
'It has been replaced with "xpack.monitoring.elasticsearch.ssl.verificationMode"'); | ||
}, | ||
(settings, log) => { | ||
if (has(settings, 'report_stats')) { | ||
log('Config key "xpack.monitoring.report_stats" is deprecated and will be removed in 7.0. ' + | ||
'Use "xpack.xpack_main.telemetry.enabled" instead.'); | ||
'Use "xpack.xpack_main.telemetry.enabled" instead.'); | ||
} | ||
}, | ||
(settings, log) => { | ||
const clusterAlertsEnabled = get(settings, 'cluster_alerts.enabled'); | ||
const emailNotificationsEnabled = clusterAlertsEnabled && get(settings, 'cluster_alerts.email_notifications.enabled'); | ||
if (emailNotificationsEnabled && !get(settings, CLUSTER_ALERTS_ADDRESS_CONFIG_KEY)) { | ||
log(`Config key "${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}" will be required for email notifications to work in 7.0."`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm running this PR branch and trying to get this log to show up, but I'm not seeing it. I see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I discovered while putting this PR together that the depreciation system only passes through the settings that are explicitly defined in your To say another way, you'll see this warning if and only if you define the following in your xpack.monitoring.cluster_alerts.enabled: true
xpack.monitoring.cluster_alerts.email_notifications.enabled: true This is what led me to implement the other deprecation warning, so that it would be more obvious/helpful to ordinary installations that don't redefine default configuration settings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good to know, thanks! |
||
} | ||
}, | ||
]; | ||
|
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.
We probably want to keep a similar name as the setting getting deprecated. Maybe
default_admin_email
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.
I considered
default_admin_email
, but to me, that implies that the email address can be overridden somewhere else. If I understand it correctly, Monitoring doesn't allow the email address to be changed for cluster alerts.The
xpack:defaultAdminEmail
advanced setting makes sense as a default option when creating new watcher jobs through the UI, because it really is the default setting, which the user can change.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.
Interesting, I didn't realize Watcher UI uses this setting!
I'm OK with how you had it.