-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Composable templates] Table to list templates #67282
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
191e9e4
to
c4f7aa6
Compare
@elasticmachine merge upstream |
There was a problem hiding this 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
toindex_templates
? - Since we have two tables now, what do you think about changing the default page size to
10
instead of20
? 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.
x-pack/plugins/index_management/server/routes/api/templates/register_create_route.ts
Outdated
Show resolved
Hide resolved
|
||
return ( | ||
<> | ||
<EuiBadge color={getColor(mappings)}>M</EuiBadge> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 @@ | |||
*/ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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', () => ({ |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 @@ | |||
/* |
There was a problem hiding this comment.
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 @@ | |||
/* |
There was a problem hiding this comment.
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}` |
There was a problem hiding this comment.
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}` : ''
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]>
Thanks for the review @alisonelizabeth !
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? |
I made the changes you recomended @alisonelizabeth , can you have another look? thanks! |
There was a problem hiding this 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.
Thanks for the review @alisonelizabeth and finding the regression. I made the fix. Will merge as soon as CI gets green 🤞 |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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. |
Co-authored-by: Alison Goryachev <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: spalger <[email protected]>
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