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

[Composable templates] Table to list templates #67282

Merged
merged 20 commits into from
Jun 4, 2020

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented May 25, 2020

This PR adds the client and server code change to support listing composable index templates in a table on top of the legacy index template.

They are no actions on the table items (view, delete, edit, clone), we can simply view the list of index templates V2.

How to test

  • Go to the index templates tab in index management and see the empty table on top of the legacy table
  • There is a "View" dropdown that allows you to toggle on/off the composable templates table and the system templates.
  • Open console in a separate tab (so you can easily update the template and reload the templates in the UI to see the changes).
  • Create a component template
PUT /_component_template/comp-1
{
   "template": {
      "mappings": {
        "properties": {
          "title": {
          "type": "text"
        }
        }
      }
    }
}
  • Then create an index template that is composed of the component template
PUT _index_template/my-v2-format
{
  "index_patterns": ["pat*"],
  "priority": 2,
  "version": 1,
  "composed_of": ["comp-1"],
  "template": {
    "mappings": {
      "properties": {
        "title": {
          "type": "text"
        }
      }
    },
    "settings": {
      "index": {
        "lifecycle": {
          "name": "my-policy",
          "rollover_alias": "some-alias"
        }
      }
    }
  }
}
  • Go back to the index template tab and verify that the table display correctly the newly created index template

Screenshot 2020-05-28 at 15 10 50

Screenshot 2020-05-28 at 15 11 00

Screenshot 2020-05-28 at 15 11 14

@sebelga sebelga added Feature:Index Management Index and index templates UI Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.0.0 labels May 26, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@sebelga sebelga force-pushed the feature/index-template/table-v2 branch from 191e9e4 to c4f7aa6 Compare May 29, 2020 10:21
@sebelga sebelga added the v7.9.0 label May 29, 2020
@sebelga sebelga marked this pull request as ready for review May 29, 2020 12:51
@sebelga sebelga requested a review from a team as a code owner May 29, 2020 12:51
@sebelga sebelga requested a review from alisonelizabeth May 29, 2020 13:06
@sebelga sebelga added the release_note:skip Skip the PR/issue when compiling release notes label May 29, 2020
@sebelga
Copy link
Contributor Author

sebelga commented Jun 1, 2020

@elasticmachine merge upstream

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Great work @sebelga!

Tested locally. Everything worked as expected.

I left a few comments in the code. I also had a few other general comments/questions:

  • With the introduction of component templates, I wonder if we should change the UI and API routes from templates to index_templates?
  • Since we have two tables now, what do you think about changing the default page size to 10 instead of 20? I didn't test it, but I imagine if you had 20 composable templates, you would have to scroll to see the legacy templates.
  • It felt a little weird to me having one filter to view composable templates and system templates, since the system filter applies to both tables, whereas selecting composable hides or removes the one table. I'd be interested to see if design has any feedback on this though.


return (
<>
<EuiBadge color={getColor(mappings)}>M</EuiBadge>
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a tooltip to these, so it's a little more clear what each one represents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 👍

@@ -5,60 +5,34 @@
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding tests for the serialization functions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to have it covered by integration. In this case the API integration tests. They actually caught a few errors in these handlers when I updated them and helped me to fix them.

@@ -22,6 +31,42 @@ const removeWhiteSpaceOnArrayValues = (array: any[]) =>

jest.mock('ui/new_platform');

// jest.mock('@elastic/eui', () => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be removed?

defaultMessage: 'Aliases',
field: 'hasMappings',
name: i18n.translate('xpack.idxMgmt.templateList.table.overridesColumnTitle', {
defaultMessage: 'Overrides',
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Is "Overrides" the right term to use here? As we are just checking whether mappings/settings/aliases exist, it may or may not be an override (if I understand correctly).

Copy link
Contributor Author

@sebelga sebelga Jun 3, 2020

Choose a reason for hiding this comment

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

Good point, I added it as a feedback to go back to in #68056

@@ -0,0 +1,295 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this file was just moved into the legacy_templates directory, so only did a light review here.

@@ -0,0 +1,327 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this file was just moved into the legacy_templates directory, so only did a light review here.

return encodeURI(
`${BASE_PATH}edit_template/${encodeURIComponent(encodeURIComponent(name))}?v=${formatVersion}`
`${BASE_PATH}edit_template/${encodeURIComponent(encodeURIComponent(name))}?legacy=${isLegacy}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since legacy is marked as optional, should this be something like:

isLegacy ? `?legacy=${isLegacy}` : ''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be a cleaner URL yes, but the logic would be the same as we would get

?legacy=undefined

So checking for

const isLegacy = queryParam.legacy === 'true'; 

would give false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change to this:

?legacy=${isLegacy === true}`

…gister_create_route.ts

Co-authored-by: Alison Goryachev <[email protected]>
@sebelga
Copy link
Contributor Author

sebelga commented Jun 3, 2020

Thanks for the review @alisonelizabeth !

With the introduction of component templates, I wonder if we should change the UI and API routes from templates to index_templates?

Good point, I will update the endpoints.

For the rest of your concerns, I think it would be better to not block this PR on these decisions as they might not be relevant later. I created this issue #68056 to keep track of your feedback and revisit it later. Is that OK for you?

@sebelga sebelga requested a review from alisonelizabeth June 3, 2020 13:15
@sebelga
Copy link
Contributor Author

sebelga commented Jun 3, 2020

I made the changes you recomended @alisonelizabeth , can you have another look? thanks!

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes @sebelga!

I noticed a small regression with spacing of the badges. Can you take a look? Otherwise, LGTM. Going to approve to not block you.

Screen Shot 2020-06-03 at 2 06 34 PM

@sebelga
Copy link
Contributor Author

sebelga commented Jun 4, 2020

Thanks for the review @alisonelizabeth and finding the regression. I made the fix. Will merge as soon as CI gets green 🤞

Screenshot 2020-06-04 at 17 11 01

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@sebelga sebelga merged commit c77e9bf into elastic:master Jun 4, 2020
@sebelga sebelga deleted the feature/index-template/table-v2 branch June 4, 2020 20:34
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 8, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 9, 2020
sebelga added a commit that referenced this pull request Jun 9, 2020
Co-authored-by: Alison Goryachev <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: spalger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Index Management Index and index templates UI release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants