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

[Discover] Migrate server side saved object from data to discover plugin #70342

Merged
merged 8 commits into from
Jul 6, 2020
Merged

[Discover] Migrate server side saved object from data to discover plugin #70342

merged 8 commits into from
Jul 6, 2020

Conversation

kertal
Copy link
Member

@kertal kertal commented Jun 30, 2020 •
edited
Loading

Summary

Migrate the server side saved object of Discover's saved search and it's registration, from the data plugin, to the discover plugin.
Welcome home buddy, we missed you 🌷 .

@kertal kertal self-assigned this Jun 30, 2020
@kertal kertal added the Feature:Discover Discover Application label Jun 30, 2020
@kertal kertal marked this pull request as ready for review July 1, 2020 08:07
@kertal kertal requested a review from a team July 1, 2020 08:07
@kertal kertal requested a review from a team as a code owner July 1, 2020 08:07
@kertal kertal added release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jul 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

stratoula
stratoula approved these changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM, also tested it manually on chrome :)

flash1293
flash1293 reviewed Jul 1, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks fine to me overall, but the saved_objects folder structure should be kept even if there is just a single definition to follow the convention

@kertal
Copy link
Member Author

kertal commented Jul 1, 2020

Looks fine to me overall, but the saved_objects folder structure should be kept even if there is just a single definition to follow the convention

thx for the reminder, @flash1293, forgot about the convention, changed that 👍

@kertal kertal requested a review from flash1293 July 1, 2020 09:31
flash1293
flash1293 approved these changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, only code review

@kertal
Copy link
Member Author

kertal commented Jul 1, 2020

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Jul 1, 2020

@elasticmachine merge upstream

lizozom
lizozom approved these changes Jul 2, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Looks great.
Haven't checked out.

Would recommend replacing the search saved object name with something less generic

src/plugins/discover/server/saved_objects/search.ts Outdated

export const searchSavedObjectType: SavedObjectsType = {
export const search: SavedObjectsType = {
Copy link
Contributor

Choose a reason for hiding this comment

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

//nit

IMO the name is waaay too generic ;-)

Copy link
Member Author

@kertal kertal Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@lizozom i do agree, but I've named it what I was thought is the convention, and it's imported just once to register :) https://github.com/elastic/kibana/blob/master/src/core/CONVENTIONS.md#saved-objects-types
should I go back or celebrate minus of 15 characters here, or should I name it s to celebrate 20chars, and definitely break the convention?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lizozom not that I rolled I've renamed this, since the less generic, the better it's searchable

@kertal
Revert change of variable name
37241ac
@kertal
Merge branch 'kertal-pr-2020-06-30-migrate-search_saved_object-to-dis…
1a74c49 
…cover' of github.com:kertal/kibana into kertal-pr-2020-06-30-migrate-search_saved_object-to-discover
@kertal
Copy link
Member Author

kertal commented Jul 6, 2020

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@kertal
Copy link
Member Author

kertal commented Jul 6, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@kertal kertal merged commit 68ebd04 into elastic:master Jul 6, 2020
@kertal kertal mentioned this pull request Jul 6, 2020
Merged
kertal added a commit to kertal/kibana that referenced this pull request Jul 6, 2020
@kertal
[Discover] Migrate server side saved object from data to discover plu…
7ebb26e 
…gin (elastic#70342)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 6, 2020
@gmmorris
Merge branch 'master' into actions/feature
6a2b64d 
* master:
  [APM] Fix flaky e2e’s (elastic#70790)
  [Discover] Migrate server side saved object from data to discover plugin (elastic#70342)
  [APM] Update docs on running API tests (elastic#70765)
  test: 💍 delete a flaky test (elastic#70785)
  [Security Solution] Refactor GlobalTime to useGlobalTime hook and cle… (elastic#69345)
  Remove IE11 mention from PR template [skip ci] (elastic#70486)
  [GS] add savedObjects result provider (elastic#68619)
  remove snapshot from disabled test suite. (elastic#70769)
kertal added a commit that referenced this pull request Jul 6, 2020
@kertal
[Discover] Migrate server side saved object from data to discover plu…
1a931c3 
…gin (#70342) (#70798)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 7, 2020
@gmmorris
Merge branch 'actions/feature' into actions/feature-and-rbac
22efe49 
* actions/feature:
  improved copy
  [APM] Fix flaky e2e’s (elastic#70790)
  [Discover] Migrate server side saved object from data to discover plugin (elastic#70342)
  [APM] Update docs on running API tests (elastic#70765)
  test: 💍 delete a flaky test (elastic#70785)
  [Security Solution] Refactor GlobalTime to useGlobalTime hook and cle… (elastic#69345)
  Remove IE11 mention from PR template [skip ci] (elastic#70486)
  [GS] add savedObjects result provider (elastic#68619)
  remove snapshot from disabled test suite. (elastic#70769)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@lizozom lizozom lizozom approved these changes

@flash1293 flash1293 flash1293 approved these changes

Assignees

@kertal kertal

Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kertal @elasticmachine @kibanamachine @flash1293 @lizozom @stratoula