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

[EPM] Change PACKAGES_SAVED_OBJECT_TYPE id #62818

Merged
merged 3 commits into from
Apr 9, 2020
Merged

[EPM] Change PACKAGES_SAVED_OBJECT_TYPE id #62818

merged 3 commits into from
Apr 9, 2020

Conversation

neptunian
Copy link
Contributor

@neptunian neptunian commented Apr 7, 2020 •
edited
Loading

Changes PACKAGES_SAVED_OBJECT_TYPE id from {packageName}-{packageVersion} to {packageName}. We can only have one package installed at a time and separating the package and version make handling versions easier.

  • enables being able to update a saved object package to a new version. otherwise I would need to create and delete a saved object, and since I can't do this atomically, there would be a point where 2 or no packages are in saved objects (this is an edge case i'm not sure we need to worry about but it is an incorrect state)
  • makes it easier to fetch the installed package by id instead of searching through saved objects for the matching name attribute. During a package update we can't currently get by id because the id will be different if the version is in the id.
  • if we end up supporting multiple versions installed at once, in the future, this might not be as convenient. With this structure they could be nested under the package name. i'm not sure how the concept of "updating" would work with multiple versions, or that it would work with our current indexing strategy so I'm not sure that's something to be thinking about at the moment.

Screen Shot 2020-04-07 at 7 04 48 PM

How to test

Install a package and view your saved objects. Like in the image, the id has changed.

@neptunian neptunian added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project v7.8.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Apr 7, 2020
@neptunian neptunian requested a review from a team April 7, 2020 15:32
@neptunian neptunian self-assigned this Apr 7, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@neptunian neptunian assigned jfsiii and unassigned jfsiii Apr 7, 2020
@neptunian neptunian requested a review from jfsiii April 7, 2020 15:50
nchaulet
nchaulet approved these changes Apr 7, 2020
View reviewed changes
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@neptunian
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@skh
Copy link
Contributor

skh commented Apr 8, 2020

The change looks logical to me, but I wasn't involved in the initial implementation -- @jfsiii maybe you could comment?

Also (@neptunian ), could you add a description how this PR can be tested?

jfsiii
jfsiii approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@jfsiii jfsiii left a comment

Choose a reason for hiding this comment

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

TL;DR; :shipit: because even if we revert back to the old naming scheme, the hard work of updating all the call sites to use name & version has been done. We would just join them at one or two locations instead reverting those sites back to one parameter.

For some context, iirc, the initial compound key was because of the only one installed version at a time and updates were something for "down the road".

I'm 👍 for proceeding with this and seeing where it goes.

@ruflin
Copy link
Member

ruflin commented Apr 9, 2020

I'm good with moving forward with this. Also +1 on having 1 location where we build the id so if we change our mind in the future, easy to change.

One followup question that should not block this PR: During an upgrade we install some assets first (ingest pipeline) and remove it later. How will this work with just having one state? What if Kibana crashes or something goes wrong in the middle of an installation where we have old and new assets around?

@neptunian neptunian merged commit a0c247b into elastic:master Apr 9, 2020
@neptunian neptunian mentioned this pull request Apr 9, 2020
Merged
neptunian added a commit to neptunian/kibana that referenced this pull request Apr 9, 2020
@neptunian @elasticmachine
[EPM] Change PACKAGES_SAVED_OBJECT_TYPE id (elastic#62818)
8ae80c0 
* changed PACKAGES_SAVED_OBJECT_TYPE id from packageName-version to packageName

* change references to keys to package and version

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 9, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into es_ui_shared/…
bf57be0 
…chore/put-all-xjson-together

* 'master' of github.com:elastic/kibana:
  [EPM] Update UI copy to use `integration` (elastic#63077)
  [NP] Inline buildPointSeriesData and buildHierarchicalData dependencies (elastic#61575)
  [Maps] create NOT EXISTS filter for tooltip property with no value (elastic#62849)
  [Endpoint] Add link to Logs UI to the Host Details view (elastic#62852)
  [UI COPY] Fixes typo in max_shingle_size for search_as_you_type (elastic#63071)
  [APM] docs: add alerting examples for APM (elastic#62864)
  [EPM] Change PACKAGES_SAVED_OBJECT_TYPE id (elastic#62818)
  docs: fix rendering of bulleted list (elastic#62855)
  Exposed AddMessageVariables as separate component (elastic#63007)
  Add Data - Adding cloud reset password link to cloud instructions (elastic#62835)
  [ML] DF Analytics:  update memory estimate after adding exclude fields (elastic#62850)
  [Table Vis] Fix visualization overflow (elastic#62630)
  [Endpoint][EPM] Endpoint depending on ingest manager to initialize (elastic#62871)
  [Remote clusters] Fix flaky jest tests (elastic#58768)
  [Discover] Hide time picker when an indexpattern without timefield is selected (elastic#62134)
  Move search source parsing and serializing to data (elastic#59919)
  [ML] Functional tests - stabilize typing in mml input (elastic#63091)
  [data.search.aggs]: Clean up TimeBuckets implementation (elastic#62123)
  [ML] Functional transform tests - stabilize source selection (elastic#63087)
  add embed flag to saved object url as well (elastic#62926)

# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/es_index.tsx
neptunian added a commit that referenced this pull request Apr 9, 2020
@neptunian @elasticmachine
[EPM] Change PACKAGES_SAVED_OBJECT_TYPE id (#62818) (#63140)
e34cf6c 
* changed PACKAGES_SAVED_OBJECT_TYPE id from packageName-version to packageName

* change references to keys to package and version

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfsiii jfsiii jfsiii approved these changes

@nchaulet nchaulet nchaulet approved these changes

Assignees

@neptunian neptunian

Labels
Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@neptunian @elasticmachine @kibanamachine @skh @ruflin @jfsiii @nchaulet