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

[Monitoring] Added deprecated tag to the advanced settings for xPack:defaultAdminEmail #70280

Merged
merged 10 commits into from
Jul 7, 2020

Conversation

igoristic
Copy link
Contributor

Fixes: #25439

This PR adds deprecation tag to the Advanced Settings config option, and also indicates the change in the docs

@igoristic igoristic added enhancement New value added to drive a business result Team:Monitoring Stack Monitoring team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Jun 30, 2020
@igoristic igoristic requested a review from a team June 30, 2020 01:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring (Team:Monitoring)

@igoristic igoristic removed the v8.0.0 label Jun 30, 2020
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for the quick turnaround here.

The other thing this PR needs to do is add back in the functionality lost in the 7.7 NP migration PR, specifically the logic in https://github.com/elastic/kibana/blob/7.5/x-pack/legacy/plugins/monitoring/server/kibana_monitoring/collectors/get_settings_collector.js#L21. It's been changed in 7.7: https://github.com/elastic/kibana/blob/7.7/x-pack/plugins/monitoring/server/kibana_monitoring/collectors/get_settings_collector.ts#L15 in a breaking way (which was my bad there) and that needs to go back to the way it was.

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

I think we also need to make changes to monitoring/deprecations.ts so the specific deprecation block looks like:

(config, fromPath, logger) => {
      const clusterAlertsEnabled = get(config, 'monitoring.cluster_alerts.enabled', true);
      const emailNotificationsEnabled =
        clusterAlertsEnabled &&
        get(config, 'monitoring.cluster_alerts.email_notifications.enabled', true);
      const updatedKey = get(config, `monitoring.${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}`);
      const legacyKey = get(config, `xpack.monitoring.${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}`);
      if (emailNotificationsEnabled && !updatedKey && !legacyKey) {
        logger(
          `Config key [${fromPath}.${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}] will be required for email notifications to work in 7.0."`
        );
      }
      return config;
    },

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

Looking good! One small request and then I think we're g2g!

get(config, 'monitoring.cluster_alerts.email_notifications.enabled', true);
const updatedKey = get(config, `monitoring.${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}`);
const legacyKey = get(config, `xpack.monitoring.${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}`);
if (emailNotificationsEnabled && !updatedKey && !legacyKey) {
logger(
`Config key [${fromPath}.${CLUSTER_ALERTS_ADDRESS_CONFIG_KEY}] will be required for email notifications to work in 7.0."`
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update this to 8.0 too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM!

@igoristic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@igoristic igoristic removed the v7.7.0 label Jul 7, 2020
@igoristic igoristic merged commit 58e64af into elastic:7.x Jul 7, 2020
@igoristic igoristic deleted the deprecate-default-email branch July 7, 2020 19:45
igoristic added a commit that referenced this pull request Jul 8, 2020
…defaultAdminEmail (#70280) (#71013)

* Added deprecated tag to the advanced settings

* Addressed code feedback

* Code feedback

* Code feedback

* Fixed version

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@igoristic
Copy link
Contributor Author

Backport:
7.8: a7226bb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes Team:Monitoring Stack Monitoring team v7.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants