Skip to content

Conversation

@chrisronline
Copy link
Contributor

Fixes #22107

This PR ensures that even if the kibana settings collector returns nothing, we still return an identical structure in the api response as if the kibana settings collector did return something.

To test:

  1. Ensure you do not have a default_admin_email set in advanced settings within Kibana.
  2. Curl against the /api/settings endpoint
  3. Verify the response structure contains the xpack: { default_admin_email: null } section.
  4. Set an email in the default_admin_email within advanced settings
  5. Curl against the /api/settings endpoint
  6. Verify the structure is identical but with the inputted value as the email instead of null.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ycombinator
Copy link
Contributor

@chrisronline Conflicts!

@ycombinator
Copy link
Contributor

ycombinator commented Aug 22, 2018

The functionality of the API as described in the test steps is good. However, with this change, the API will return a slightly different response than what the internal monitoring code in Kibana would ship to Monitoring. I believe the latter would never ship a document to Monitoring if the value of the email address is null, because of the state maintenance that is happening via the shouldUseNull variable. Ideally both the API and the internal monitoring code would have the same end results for consistency. Also, it seems a bit hacky that the API route code has to futz with the actual result of settingsCollector.fetch() before sending it out.

I wonder if we can get rid of the shouldUseNull state behavior and solve both problems: consistency and avoiding the futzing on the caller's end.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

default_admin_email: email
}
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Since the collector constructor spreads in any property given to it, we can make this function a part of the settings collector instance.

something like:

collectorSet.makeStatsCollector({	
  type: ...,
  async fetch: ...,
  getEmailValueStructure: email => ({
    xpack: { default_admin_email: email }
  })
})

In the line below where getEmailValueStructure is used in the fetch, it can use this.getEmailValueStructure(defaultAdminEmail)

import { wrap as wrapError } from 'boom';
import { KIBANA_SETTINGS_TYPE } from '../../../../../monitoring/common/constants';
import { getKibanaInfoForStats } from '../../../../../monitoring/server/kibana_monitoring/lib';
import { getEmailValueStructure } from '../../../../../monitoring/server/kibana_monitoring/collectors/get_settings_collector';
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to not have this import when getEmailValueStructure is a method of settingsCollector

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

A suggestion to make the new function more handy, and not need the new import

@chrisronline
Copy link
Contributor Author

@tsullivan Great suggestions! Updated now!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

I pulled down the branch and tested the API; refreshing a few times to ensure the same object is returned each time

@chrisronline chrisronline merged commit db089ea into elastic:master Aug 23, 2018
@chrisronline chrisronline deleted the monitoring/fix/22107 branch August 23, 2018 13:04
chrisronline added a commit to chrisronline/kibana that referenced this pull request Aug 23, 2018
…admin_email (elastic#22220)

* If the settings collector returns nothing, ensure the settings api still returns a null value for default_admin_email

* Update test

* Feedback from PR
chrisronline added a commit that referenced this pull request Aug 23, 2018
…admin_email (#22220) (#22289)

* If the settings collector returns nothing, ensure the settings api still returns a null value for default_admin_email

* Update test

* Feedback from PR
@chrisronline
Copy link
Contributor Author

Backport:

6.x: 4fe840e

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants