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] move MapSavedObject type out of telemetry #60127

Merged
merged 5 commits into from
Mar 16, 2020
Merged

[Maps] move MapSavedObject type out of telemetry #60127

merged 5 commits into from
Mar 16, 2020

Conversation

nreese
Copy link
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 [Deprecated-Use Team:Presentation]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
Contributor

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
Contributor

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
Contributor Author

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
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
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

@nreese
Copy link
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
Contributor

💚 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
Contributor

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
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
Contributor

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
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
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

@kindsun kindsun kindsun approved these changes

@mshustov mshustov mshustov approved these changes

Assignees
No one assigned
Labels
backported chore [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nreese @elasticmachine @kibanamachine @spalger @kindsun @mshustov