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

Refactor attribute service #78414

Merged
merged 13 commits into from
Oct 6, 2020

Conversation

majagrubic
Copy link
Contributor

@majagrubic majagrubic commented Sep 24, 2020

Summary

First iteration of addressing: #77505

What this PR does:

  1. removes savedObjectsClient dependency from AttributeService
  2. makes all methods that previously used savedObjectsClient mandatory for attribute service - ie. saveMethod, unwrapMethod, checkForDuplicateTitle
  3. implements the above-mentioned methods in the corresponding embeddable factories

What this PR doesn't do:

  1. Removes Attribute Service from Dashboard plugin -> since this change is already quite big, I thought it would be best to leave it for the next PR

Would love to get early feedback on this and if this is how you think it should work.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@majagrubic majagrubic changed the title Extract attribute service from Dashboard plugin Refactor attribute service Oct 1, 2020
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Everything here looks great so far. Tested locally on chrome and it LGTM. Code changes also look good. I think the functional test failures are due to committing the dashboard feature flag.

@majagrubic
Copy link
Contributor Author

@ThomThomson thanks for the review, I did indeed leave the flag option on while in draft mode, to make reviewing easier. Changed that now.

@majagrubic majagrubic marked this pull request as ready for review October 2, 2020 09:36
@majagrubic majagrubic requested a review from a team October 2, 2020 09:36
@majagrubic majagrubic requested a review from a team as a code owner October 2, 2020 09:36
@majagrubic
Copy link
Contributor Author

@elasticmachine merge upstream

@majagrubic majagrubic added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Oct 5, 2020
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
dashboard 570.2KB 569.3KB -984.0B
lens 76.0KB 77.0KB +1022.0B
visualizations 272.2KB 273.3KB +1.1KB
total +1.1KB

History

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

@majagrubic majagrubic merged commit 989e9c9 into elastic:master Oct 6, 2020
@majagrubic majagrubic deleted the extract-attribute-service branch October 6, 2020 07:31
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 6, 2020
* master: (85 commits)
  Refactor attribute service (elastic#78414)
  [APM] Add default message to alerts. (elastic#78930)
  [Discover] Modify columns and sort when switching index pattern (elastic#74336)
  Document ts project references setup (elastic#78586)
  build all ts refs in single kbn:bootstrap (elastic#79438)
  [TSVB] Allow string fields on value count aggregation (elastic#79267)
  [SECURITY SOLUTION] Investigate EQL signal in timeline (elastic#79049)
  [Fleet] Add loading spinner to page headers (elastic#79568)
  [Security Solution][Resolver] Resolver query panel load more (elastic#79160)
  Add type row to monitor detail page. (elastic#79556)
  Remove license refresh from setup (elastic#79518)
  [docker] add reporting fonts (elastic#74806)
  [SECURITY_SOLUTION][ENDPOINT] Add info about trusted apps to the page subtitle + create flyout (elastic#79558)
  Trim Hash value before validating it (elastic#79545)
  Warn users when security is not configured (elastic#78545)
  update copy styling (elastic#79313)
  Update dependency @elastic/charts to v23.1.1 (elastic#78459)
  Introduce geo-threshold alerts (elastic#76285)
  elastic#76920 Show base breadcrumb when there is an error booting the app (elastic#79571)
  remove react-intl from kibana and keep it inside only i18n package (elastic#78956)
  ...
majagrubic pushed a commit that referenced this pull request Oct 6, 2020
* Making saveMethod mandatory in attribute service

* Making unwrap method mandatory

* Making book embeddable respect new attribute service

* Remove savedObjectsClient from attribute service

* Add checkForDuplicateTitle method to book embeddable

* Make options mandatory on attribute service

* Changing Lens attribute service

* Somw more typescript fixes

* Fixing attribute service typescript and tests

* Fixing typescript errors

* Unsetting feature flag

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants