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

Load configuration from EMS-metadata in region-maps #70888

Merged

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Jul 6, 2020

Region-maps would rely on the visState to load the human-readable name and attribution of the EMS. While reading out settings of the visState is the canonical approach in Visualize for settings configured in the UX (e.g. metrics and buckets), region maps should not do this for the layer configurations that get rendered to the screen as-is.

This PR makes two changes:

  • For EMS-layers, the relevant metadata should be loaded from the EMS-metadata.
  • For self-hosted layers, the relevant metadata should be loaded from the settings in the kibana.yml. Additionally, the attribution-string (maps.regionmap.layers[].attribution) gets URL-escaped. This parameter is only allowed to be a text-string, as per the existing documentation (https://www.elastic.co/guide/en/kibana/current/settings.html).

Blocked by #70992. When that merges, can add appropriate test coverage.

…ad of relying on the vis-state.

For EMS-layers, load from EMS-metadata. For self-hosted layers, load from the kibana.yml config.
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@thomasneirynck thomasneirynck changed the title Load configuration from EMS-meta in Load configuration from EMS-metadata in region-maps Jul 6, 2020
@thomasneirynck
Copy link
Contributor Author

@elasticmachine merge upstream

@thomasneirynck
Copy link
Contributor Author

sorry for the test-noise, but I merged in #70992, so I can verify the fixes. When #70992 merges, this diff of this PR will condense again.

Copy link
Contributor

@kindsun kindsun left a comment

Choose a reason for hiding this comment

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

Thanks for the quick diagnosis, fix and minor clean-up. Changes lgtm! Tested locally and was able to repro the error on master and confirm the issue did not exist on this branch.

  • code review
  • tested locally in chrome and firefox

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Changes look great to me as well, thanks for the quick turnaround and collaboration on this. Tested locally on Chrome.

LGTM pending the merge of #70992 and subsequent green CI run 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@thomasneirynck thomasneirynck merged commit 9cf1dec into elastic:master Jul 8, 2020
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 8, 2020
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 8, 2020
thomasneirynck added a commit that referenced this pull request Jul 9, 2020
thomasneirynck added a commit that referenced this pull request Jul 9, 2020
…70888) (#71162)

Manual backport to account for accumulated refactors. 6.8 branch predates angular->react conversion, does not use the external ems-client package, and Visualize was still not using expression-pipeline.
@LeeDr
Copy link
Contributor

LeeDr commented Aug 6, 2020

I removed v6.8.12 label because it appears this was already in v6.8.11.
I removed v7.8.2 label because it appears this was already in v7.8.1.

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.

6 participants