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

Confirm Maps App Enabled and License defined before using in App #27607

Merged
merged 1 commit into from
Dec 21, 2018

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Dec 20, 2018

Resolves #27544. The license info was being assigned correctly, it just hadn't been defined yet. The pieces were in place to wait until the license info had been updated before leveraging the license info, they just hadn't been "hooked up" yet. Also added in a condition that the map app should be enabled.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kindsun
Copy link
Contributor Author

kindsun commented Dec 20, 2018

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor

nreese commented Dec 20, 2018

I think you will need to rebase with master to get CI to pass (once #27608 is merged). CI failure below sounds like #27608

fail: "apis xpack_main Telemetry /api/telemetry/v1/clusters/_stats with monitoring disabled should pull local stats and validate fields"
18:57:58                  │ 
18:57:58                  │       Error: expected [ 'cluster_name',
18:57:58                  │   'cluster_stats.cluster_uuid',
18:57:58                  │   'cluster_stats.indices.completion.size_in_bytes',

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

@kindsun kindsun requested a review from a team as a code owner December 20, 2018 20:20
@kindsun kindsun force-pushed the fix/mapsLicenseCheckTiming branch from 82b1c60 to 310da22 Compare December 20, 2018 20:53
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kindsun
Copy link
Contributor Author

kindsun commented Dec 20, 2018

jenkins, test this

@nreese
Copy link
Contributor

nreese commented Dec 20, 2018

Just thought of one thing that would improve readability. Instead of burying everything under the if clause, maybe just return when plugin is not enabled

if (!gisEnabled) {
  return
}

...other logic

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

Looks like the canvas team was tapped due to a bad merge. Since this doesn't involve Canvas at all anymore, my approval here is simply to unblock your PR.

@kindsun kindsun merged commit 074a427 into elastic:master Dec 21, 2018
kindsun added a commit to kindsun/kibana that referenced this pull request Dec 28, 2018
…ssing license info through to maps app (elastic#27607)

# Conflicts:
#	x-pack/plugins/gis/server/routes.js
kindsun added a commit that referenced this pull request Jan 3, 2019
…ssing license info through to maps app (#27607) (#27842)

# Conflicts:
#	x-pack/plugins/gis/server/routes.js
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