Skip to content

Add multiple index support to saved objects#35747

Merged
mattapperson merged 15 commits intoelastic:masterfrom
mattapperson:so-index
Jun 3, 2019
Merged

Add multiple index support to saved objects#35747
mattapperson merged 15 commits intoelastic:masterfrom
mattapperson:so-index

Conversation

@mattapperson
Copy link
Copy Markdown
Contributor

@mattapperson mattapperson commented Apr 29, 2019

This PR is a WIP from a code standpoint, but am requesting a functional review. Items still needed:

  • Cleanup around mapping / migration iteration
  • Support deleteByNamespace

Summary

This is an initial pass at adding support to Saved Objects to allow document schema definitions to define a custom index name for something other .kibana

Note: schema prop name of indexPattern is used even though a "pattern" is not yet supported as a future/followup PR should add support for a pattern

API

To make use of this new feature all you must do is define the index in the savedObjectSchemas values of your plugins init like so:

{
    version: '8.2.3',
    uiExports: {
      savedObjectMappings: [
        {
          pluginId: 'testtype',
          properties: {
            testtype: {
              properties: {
                name: { type: 'keyword' },
              },
            },
          },
        },
        {
          pluginId: 'testtype2',
          properties: {
            testtype2: {
              properties: {
                name: { type: 'keyword' },
              },
            },
          },
        },
      ],
      savedObjectSchemas: {
        testtype: {
          indexPattern: 'other-index',
        },
      },
    }

Testing

This PR is small, but affects a LOT of surface area so I am breaking down areas that need special attention. (please comment if I missed something)
While much if not all of this is covered by automated testing, the fact that SOs is critical to Kibana function I think demands a little extra special attention

  • Works with spaces (@kobelb)
  • Works with feature controls (@kobelb)
  • SO API
  • SO hidden features
  • Migrations console optput

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@kobelb
Copy link
Copy Markdown
Contributor

kobelb commented Apr 29, 2019

I don't see any issues with the approach in general preventing Spaces/Security from functioning properly.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@mattapperson
Copy link
Copy Markdown
Contributor Author

Jenkins test this

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Copy link
Copy Markdown
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Few comments but looking forward to the feature! 👍

@mikecote mikecote requested a review from a team May 9, 2019 12:48
@epixa
Copy link
Copy Markdown
Contributor

epixa commented May 9, 2019

@mattapperson Can you flesh out the details in this PR description about how this is to be used from a plugin's perspective? Sample code would be great.

@mattapperson
Copy link
Copy Markdown
Contributor Author

@epixa will do

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@mattapperson
Copy link
Copy Markdown
Contributor Author

Jenkins test this

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Copy link
Copy Markdown
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

LGTM

@mattapperson mattapperson merged commit 79a1f47 into elastic:master Jun 3, 2019
Copy link
Copy Markdown
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

I apologize for leaving an after-the-fact review, just noticed this PR after pulling master into #36829


const esOptions = {
index: this._index,
index: this._getIndexForType(type),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

type could be a string | string[] but you're only handling the string case.

}
}

async _readFromCluster(method, params) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be unused, is the idea to take advantage of it in future work?

return (
(this._schema.definition &&
this._schema.definition[type] &&
this._schema.definition[type].indexPattern) ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Although JS will let you get away with it, you're accessing private properties of a Typescript class

@rudolf
Copy link
Copy Markdown
Contributor

rudolf commented Jun 3, 2019

Because of the interdependence of these PR's I'll address this in #36829

rudolf added a commit to rudolf/kibana that referenced this pull request Jun 3, 2019
mattapperson added a commit to mattapperson/kibana that referenced this pull request Jun 3, 2019
* Fixed jest race condition. Map queries to the correct index. Run migrations on all indexes

* fix linting

* fix linting

* build index map after plugin initalization

* wait for ES to get config I guess

* add more index mapping

* update some tests

* fix one more test

* Add multi-index support to deleteByNamespace

* remove uneeded comments

* update types

* fix yarn lock and add empty object
mattapperson added a commit that referenced this pull request Jun 3, 2019
* Fixed jest race condition. Map queries to the correct index. Run migrations on all indexes

* fix linting

* fix linting

* build index map after plugin initalization

* wait for ES to get config I guess

* add more index mapping

* update some tests

* fix one more test

* Add multi-index support to deleteByNamespace

* remove uneeded comments

* update types

* fix yarn lock and add empty object
@tylersmalley
Copy link
Copy Markdown
Member

tylersmalley commented Jul 12, 2019

@mattapperson, can you add tagging for the versions which this was added in? Also, there are TODO's which were not checked off regarding testing. Was that testing completed?

@mattapperson
Copy link
Copy Markdown
Contributor Author

Yes testing was completed, I was lazy in checking the boxes.
Version tags added

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants