Skip to content

[monitoring] remove deprecated xpack:defaultAdminEmail#33603

Merged
jbudz merged 17 commits intoelastic:masterfrom
jbudz:remove/email-setting
Jun 28, 2019
Merged

[monitoring] remove deprecated xpack:defaultAdminEmail#33603
jbudz merged 17 commits intoelastic:masterfrom
jbudz:remove/email-setting

Conversation

@jbudz
Copy link
Copy Markdown
Contributor

@jbudz jbudz commented Mar 20, 2019

Deprecated in 6.5 via spaces
Closes #32707, follow for more context.

@jbudz jbudz added review Team:Monitoring Stack Monitoring team labels Mar 20, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 20, 2019

Pinging @elastic/stack-monitoring

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 20, 2019

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 20, 2019

💔 Build Failed

@chrisronline
Copy link
Copy Markdown
Contributor

chrisronline commented Mar 21, 2019

@jbudz I'm still seeing the option to configure this in the settings. Is that intended?
Screen Shot 2019-03-21 at 11 17 16 AM

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 21, 2019

💚 Build Succeeded

@jbudz
Copy link
Copy Markdown
Contributor Author

jbudz commented Mar 21, 2019

Whoa, good catch, taking a look. that capital P is throwing me off.

@jbudz jbudz removed the review label Mar 21, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 21, 2019

💚 Build Succeeded

@jbudz
Copy link
Copy Markdown
Contributor Author

jbudz commented Mar 21, 2019

It looks like it's intentional. Anything in the config document that isn't defined is loaded as 'custom'. We can delete it but I'm not sure if there's a straight forward way.

It was defined in xpack. Migrations require mappings and migrations for types to exist in the same plugin..so I had to move things, it's a little strange. I'll push it up but I'm undecided on it.

@jbudz
Copy link
Copy Markdown
Contributor Author

jbudz commented Mar 21, 2019

@tylersmalley do you have any thoughts on what migrations for the config document should look like? I'm not sure how I feel about 1dee8db

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 21, 2019

💔 Build Failed

@jbudz jbudz added the non-issue Indicates to automation that a pull request should not appear in the release notes label Mar 27, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Apr 18, 2019

💔 Build Failed

@jbudz
Copy link
Copy Markdown
Contributor Author

jbudz commented Apr 29, 2019

@chrisronline we discussed in standup, leaning towards leaving it up. There's nothing in place to remove this the right way atm, and it doesn't have any side effects. Thoughts?

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Apr 29, 2019

💚 Build Succeeded

@chrisronline
Copy link
Copy Markdown
Contributor

chrisronline commented May 10, 2019

@jbudz Yea seems fine for now then

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 10, 2019

💔 Build Failed

@chrisronline
Copy link
Copy Markdown
Contributor

chrisronline commented Jun 26, 2019

@jbudz Sorry about forgetting this. Are you just waiting for my LGTM?

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jun 26, 2019

💔 Build Failed

@jbudz
Copy link
Copy Markdown
Contributor Author

jbudz commented Jun 27, 2019

np, yep I can merge after a lgtm. I'll get master merged in.

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jun 27, 2019

💔 Build Failed

@jbudz jbudz added release_note:skip Skip the PR/issue when compiling release notes release_note:deprecation and removed non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes labels Jun 27, 2019
chrisronline
chrisronline reviewed Jun 27, 2019
View reviewed changes
x-pack/legacy/plugins/watcher/public/services/action_defaults/actions/email_action.js
function getActionDefaults() {
return {
to: config.get(DEFAULT_ADMIN_EMAIL_CONFIG_KEY)
to: ''
Copy link
Copy Markdown
Contributor

@chrisronline chrisronline Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

cc @elastic/es-ui team - this might be a feature regression - I think you are in the process of refactoring this so heads up about this change

Copy link
Copy Markdown
Contributor

@bevacqua bevacqua Jun 27, 2019

Choose a reason for hiding this comment

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

Please don't ping that team, it's not just Kibana

Copy link
Copy Markdown
Contributor

@chrisronline chrisronline Jun 27, 2019

Choose a reason for hiding this comment

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

@bevacqua Thanks for the call out. I had the wrong alias. Updated

Copy link
Copy Markdown
Contributor

@alisonelizabeth alisonelizabeth Jun 27, 2019

Choose a reason for hiding this comment

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

@chrisronline thanks for the heads up! i'll update the refactored watcher code (#35301).

Copy link
Copy Markdown
Contributor

@alisonelizabeth alisonelizabeth Jun 27, 2019

Choose a reason for hiding this comment

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

Fixed in this commit: ccecb29

Copy link
Copy Markdown
Contributor Author

@jbudz jbudz Jun 27, 2019

Choose a reason for hiding this comment

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

hey @alisonelizabeth as a heads up I (added and then) dropped the 7.x label from this, ref #33603 (comment). only 8.0 as the urgency isn't too high

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jun 27, 2019

💔 Build Failed

chrisronline
chrisronline approved these changes Jun 27, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@chrisronline chrisronline left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, once tests are passing!

@cjcenizal
Copy link
Copy Markdown
Contributor

cjcenizal commented Jun 27, 2019

Which releases are we targeting with this?

@jbudz
Copy link
Copy Markdown
Contributor Author

jbudz commented Jun 27, 2019

retest

@cjcenizal
Copy link
Copy Markdown
Contributor

cjcenizal commented Jun 27, 2019

Could we discuss this before merging? I'm concerned about introducing a breaking change in a minor, even if it's something that's been deprecated for awhile.

@jbudz jbudz removed the v7.4.0 label Jun 27, 2019
@cjcenizal cjcenizal mentioned this pull request Jun 28, 2019
Merged
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jun 28, 2019

💚 Build Succeeded

@jbudz jbudz merged commit d0f0315 into elastic:master Jun 28, 2019
@cjcenizal cjcenizal mentioned this pull request Jun 29, 2019
Merged
@alexander-marquardt
Copy link
Copy Markdown

alexander-marquardt commented Jul 29, 2019 •
edited
Loading

This setting appears to be used by many of the built-in default (pre-installed) watchers that are installed on monitoring clusters in Elastic Cloud.

Eg:

 "script" : {
              "source" : "ctx.vars.email_recipient = (ctx.payload.kibana_settings.hits.total > 0 && ctx.payload.kibana_settings.hits.hits[0]._source.kibana_settings.xpack != null) ? ctx.payload.kibana_settings.hits.hits[0]._source.kibana_settings.xpack.default_admin_email ....

I can't seem to get the above code to actually set the xpack.email_recipient in 7.2. Are the built-in watchers that rely on this field expected to be working in 7.2?

@alexander-marquardt
Copy link
Copy Markdown

alexander-marquardt commented Jul 29, 2019

I filed an issue about this: https://github.com/elastic/elasticsearch/issues/44961

@renshuki
Copy link
Copy Markdown
Contributor

renshuki commented Jun 15, 2020

Also shouldn't this be added as a Breaking Change in the Kibana doc?
If xpack.monitoring.cluster_alerts.email_notifications.email_address is not specified in kibana.yml, monitoring emails will not be sent out due to the removal.

@chrisronline
Copy link
Copy Markdown
Contributor

chrisronline commented Jun 15, 2020

@renshuki I don't see it mentioned in the breaking changes for 7.0 so that's a mistake there, but it is a breaking change and the email needs to be configured through monitoring.cluster_alerts.email_notifications.email_address (for 7.7, prior to 7.7 this config is xpack.monitoring.cluster_alerts.email_notifications.email_address)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alisonelizabeth alisonelizabeth alisonelizabeth left review comments

+2 more reviewers

@bevacqua bevacqua bevacqua left review comments

@chrisronline chrisronline chrisronline approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release_note:deprecation Team:Monitoring Stack Monitoring team v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove xpack:default_admin_email for monitoring use

8 participants

@jbudz @elasticmachine @chrisronline @cjcenizal @alexander-marquardt @renshuki @bevacqua @alisonelizabeth