Watcher notification settings Upgrade checks #36907
Conversation
|
Pinging @elastic/es-core-features |
| .map(nodeInfo -> nodeInfo.getNode().getName()) | ||
| .collect(Collectors.toList()); | ||
| if (nodesFound.size() > 0) { | ||
| return new DeprecationIssue(DeprecationIssue.Level.CRITICAL, |
There was a problem hiding this comment.
I'm concerned that, if there's both email and hipchat accounts set up insecurely, this will produce a deprecation warning for only the email account until that's resolved, then switch to warning about the hipchat account. I think we should either break this out into many separate checks, or merge the nodesFound lists before creating the DeprecationIssue.
My initial preference is for the second one, with deprecation text something like:
new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
"Watcher notification accounts' authentication settings must be defined securely",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-7.0.html" +
"#watcher-notifications-account-settings",
"nodes which have insecure notification account settings are: " + nodesFound);
Although, if you feel strongly that we should have a bunch of separate checks so we can't specify the type of account for each warning, I would be okay with that too.
| "Watcher email notifications' password settings has to be defined securely", | ||
| "https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-7.0.html" + | ||
| "#watcher-notifications-account-settings", | ||
| "nodes which have the un-secure setting are: " + nodesFound); |
There was a problem hiding this comment.
Nitpick:
| "nodes which have the un-secure setting are: " + nodesFound); | |
| "nodes which have insecure notification account settings are: " + nodesFound); |
|
Hey @gwbrown , Guess what!? I have addressed the feedback :)) Good catch! |
| @@ -50,6 +50,7 @@ private DeprecationChecks() { | |||
| NodeDeprecationChecks::gcsRepositoryChanges, | |||
| NodeDeprecationChecks::fileDiscoveryPluginRemoved, | |||
| NodeDeprecationChecks::defaultSSLSettingsRemoved | |||
There was a problem hiding this comment.
The build is failing though, you need a comma here.
Upgrade checks for deprecations in #36403
and #36736 .