Skip to content

[Maps] move MapSavedObject type out of telemetry#60127

Merged
nreese merged 5 commits intoelastic:masterfrom
nreese:map_saved_object_type
Mar 16, 2020
Merged

[Maps] move MapSavedObject type out of telemetry#60127
nreese merged 5 commits intoelastic:masterfrom
nreese:map_saved_object_type

Conversation

@nreese
Copy link
Copy Markdown
Contributor

@nreese nreese commented Mar 13, 2020 •
edited
Loading

Blended layers requires a migration and a TS migration file will require MapSavedObject. This PR moves MapSavedObject from map_telemetry into a more common location. I moved this into its own PR so the changes could be evaluated without all the noise of blended layer PR. I changed the name from IMapSavedObject to MapSavedObject because I view interfaces as describing a contract that needs an implementation while MapSavedObject is just a simple structure of properties with no implementation/methods.

@nreese nreese added chore Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 13, 2020
@nreese nreese requested a review from kindsun March 13, 2020 17:12
@nreese nreese requested a review from a team as a code owner March 13, 2020 17:12
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 13, 2020

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

kindsun
kindsun reviewed Mar 13, 2020
View reviewed changes
x-pack/plugins/maps/common/map_saved_object_type.ts
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { SavedObject } from '../../../../src/core/server';

export type MapSavedObjectAttributes = {
Copy link
Copy Markdown
Contributor

@kindsun kindsun Mar 13, 2020

Choose a reason for hiding this comment

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

Why are we changing from interface to type? Similar refs in the code use an interface: https://github.com/elastic/kibana/blob/f1272b5ffe34d7efacb359260e930b872e8e4b06/src/core/server/saved_objects/types.ts

Copy link
Copy Markdown
Contributor Author

@nreese nreese Mar 13, 2020

Choose a reason for hiding this comment

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

We have started the precedent in https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/maps/common/descriptor_types.d.ts to use type when defining something with just properties and interface when defining an actual interface that contains methods that need to be implemented.

@nreese nreese requested a review from a team as a code owner March 13, 2020 17:55
@nreese nreese requested a review from mshustov March 13, 2020 17:56
@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Mar 13, 2020 •
edited
Loading

After discussions with @restrry this PR also moves SavedObject from core/server to core/types so it can be imported from common and not cross common|server|public file boundary

kindsun
kindsun approved these changes Mar 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kindsun kindsun 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.

Maps telemetry changes lgtm! w/ green CI

  • tested locally in chrome. telemetry results compared to master
  • code review

mshustov
mshustov approved these changes Mar 16, 2020
View reviewed changes
@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Mar 16, 2020

@restrry FYI, I was getting errors in check_published_api_changes. I ran node scripts/check_published_api_changes.js --accept and committed the changes. Not sure if they are expected or not from moving the SaveObject definition.

Screen Shot 2020-03-16 at 1 09 45 PM

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 16, 2020

💚 Build Succeeded

History

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

@nreese nreese merged commit ef32611 into elastic:master Mar 16, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 17, 2020
@gmmorris
Merge branch 'master' into alerting/tls-warning
ff11eb0 
* master: (30 commits)
  [TSVB] fix text color when using custom background color (elastic#60261)
  Fix import to timefilter from in TSVB (elastic#60296)
  [NP] Get rid of usage redirectWhenMissing service (elastic#59777)
  [SIEM] Fix Timeline footer styling (elastic#59587)
  [ML] Fixes to error handling for analytics jobs and file data viz (elastic#60249)
  Give better stack traces for Unhandled Promise Rejection warnings (elastic#60235)
  resolves elastic#58905 (elastic#60120)
  Added variables button for text fields in Pagerduty component. (elastic#60189)
  adds test that action vars are rendered for alert action parms (elastic#60310)
  Closes 59786 by removing the update toast (elastic#60172)
  [EPM] Packages list tabs (elastic#60167)
  Added message variables button for Webhook body form field (elastic#60174)
  Revert "adds new test (elastic#60064)"
  [Maps] move MapSavedObject type out of telemetry (elastic#60127)
  [Reporting] Fix error handling for job handler in route (elastic#60161)
  [Endpoint] TEST: verify alerts page header says 'Alerts' (elastic#60206)
  EMT-248: implement ack resource to accept event payload to acknowledge agent actions (elastic#60218)
  Migrate dual validated range (elastic#59689)
  Embeddable triggers (elastic#58440)
  [Endpoint] Sample data generator CLI script (elastic#59952)
  ...
@nreese nreese mentioned this pull request Mar 17, 2020
Merged
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 17, 2020
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 17, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 60127 or prevent reminders by adding the backport:skip label.

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Mar 17, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 60127 or prevent reminders by adding the backport:skip label.

not sure why this pinged. backported to 7.x with #60377

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 18, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 60127 or prevent reminders by adding the backport:skip label.

@nreese
Copy link
Copy Markdown
Contributor Author

nreese commented Mar 18, 2020

@spalger How do I make these missing back port reminders stop? The PR has been backported with backport tool.

@spalger
Copy link
Copy Markdown
Contributor

spalger commented Mar 19, 2020

prevent reminders by adding the backport:skip label

Or you can use the backported label.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@kindsun kindsun kindsun approved these changes

@mshustov mshustov mshustov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backported chore release_note:skip Skip the PR/issue when compiling release notes Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.7.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nreese @elasticmachine @kibanamachine @spalger @kindsun @mshustov