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.

Sign up for GitHub

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

Jump to bottom

[Maps] use EMS v6.6 #27560

Merged
merged 11 commits into from
Dec 26, 2018
Merged

[Maps] use EMS v6.6 #27560

merged 11 commits into from
Dec 26, 2018

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Dec 20, 2018 •
edited
Loading

Closes #27559.
Closes #27582.
Closes #27611.

@thomasneirynck thomasneirynck added v7.0.0 v6.7.0 bug Fixes for quality problems that affect the customer experience [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation labels Dec 20, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@thomasneirynck thomasneirynck changed the title Should use EMS v6.6 [Maps] use EMS v6.6 Dec 20, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

nreese
nreese reviewed Dec 20, 2018
View reviewed changes
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.

How about adding some functional tests? Maybe an API intergration test that verifies the data/ems route? And then a functional test the verifies the EMS list contains vector layers

x-pack/plugins/gis/server/routes.js Outdated
});

server.route({
method: 'GET',
path: `${ROOT}/data/ems`,
handler: async (request) => {

if (!request.query.name) {
if (!request.query.id) {
console.warn('Must supply id parameters to retrieve EMS file');
Copy link
Contributor

Choose a reason for hiding this comment

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

Server messages should be logged with server.log, server.log(['warning', message);

x-pack/plugins/gis/public/shared/layers/sources/source.js
@@ -20,7 +20,7 @@ export class ASource {
this._descriptor = descriptor;
}

destory() {}
destroy() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for fixing this

@nreese
Copy link
Contributor

nreese commented Dec 20, 2018

CI failures look legitimate. All x-pack functional tests are failing

Error: Cannot find module '../../../../src/core_plugins/ems_util/common/ems_client'
14:16:55    │ debg 
14:16:55    │ERROR failure
14:16:55    │ERROR Error: Command failed: ./bin/kibana --env.name=production --logging.json=false --optimize

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nickpeihl
Copy link
Member

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

nickpeihl
nickpeihl requested changes Dec 20, 2018
View reviewed changes
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

Overall, this looks good. I'm not 100% sure this is why CI is failing but I also got this message when I ran yarn start:

log [18:57:41.137] [warning][plugin] Skipping non-plugin directory at /Users/nickpeihl/Development/Elastic/kibana/src/legacy/core_plugins/ems_util

@azasypkin
Copy link
Member

Overall, this looks good. I'm not 100% sure this is why CI is failing but I also got this message when I ran yarn start:
log [18:57:41.137] [warning][plugin] Skipping non-plugin directory at /Users/nickpeihl/Development/Elastic/kibana/src/legacy/core_plugins/ems_util

I believe it should be fixed via #27582

@thomasneirynck
Copy link
Contributor Author

fixed import, merged master

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author

jenkins test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor

nreese commented Dec 26, 2018

@thomasneirynck Why did you move all of the EMS stuff under the folder core_plugins/tile_map vs putting it into its own folder at core_plugins/ems?

nreese
nreese approved these changes Dec 26, 2018
View reviewed changes
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.

Just some minor suggestions

lgtm with green CI
Code review, tested GIS app and region map in chrome and both properly load EMS vector layers

src/legacy/core_plugins/tile_map/common/ems_client.js Outdated
);

const markdownIt = new MarkdownIt({
html: false,
linkify: true
});

/**
* x-pack plugins cannot have upstream dependencies on core/*-kibana.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of calling x-pack by name, just say something more generic like "Plugins cannot have ..."

x-pack/plugins/gis/server/routes.js Outdated
@@ -45,6 +50,8 @@ export function initRoutes(server) {
path: `${ROOT}/meta`,
handler: async () => {

// console.log('data/ems');
Copy link
Contributor

Choose a reason for hiding this comment

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

these console statements can be removed

nickpeihl
nickpeihl approved these changes Dec 26, 2018
View reviewed changes
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@thomasneirynck thomasneirynck merged commit a3b4f68 into elastic:master Dec 26, 2018
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Dec 26, 2018
@thomasneirynck
[Maps] use EMS v6.6 (elastic#27560)
aa0d2f2 
Maps plugin should use correct EMS version.
thomasneirynck added a commit that referenced this pull request Dec 27, 2018
@thomasneirynck
[Maps] use EMS v6.6 (#27560) (#27795)
004da20 
Maps plugin should use correct EMS version.
@thomasneirynck
Copy link
Contributor Author

6.x backport: #27795

@legrego legrego mentioned this pull request Feb 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nreese nreese nreese approved these changes

@nickpeihl nickpeihl nickpeihl approved these changes

@kindsun kindsun Awaiting requested review from kindsun

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v6.7.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasneirynck @elasticmachine @nreese @nickpeihl @azasypkin