-
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
[Runtime fields] Add support in index template #84184
Conversation
d311bde
to
ad8fb03
Compare
7eb5c08
to
ed4e84d
Compare
Thanks for the review @jloleysens ! I will wait on a review from the @esdocs team before merging 👍 |
@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.
Overall, looks really good! I left some suggested changes for wording, and also think we can provide more targeted URLs in a few spots. I left a comment about error handling when a script isn't provided and tagged Luca for his input.
...c/application/components/mappings_editor/__jest__/client_integration/runtime_fields.test.tsx
Outdated
Show resolved
Hide resolved
...ent/public/application/components/mappings_editor/components/runtime_fields/empty_prompt.tsx
Outdated
Show resolved
Hide resolved
...ent/public/application/components/mappings_editor/components/runtime_fields/empty_prompt.tsx
Outdated
Show resolved
Hide resolved
{ | ||
validator: emptyField( | ||
i18n.translate('xpack.runtimeFields.form.validations.scriptIsRequiredErrorMessage', { | ||
defaultMessage: 'Script must emit() a value.', |
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.
@sebelga, a script is no longer required to create a runtime field since @romseygeek merged 64981. I believe the script is still the preferred way, so perhaps we want users to create a script. But throwing in an error that forces users to create a script - when they don't have to - seems incorrect.
cc: @javanna to get his input on how we want to handle this situation.
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 catch 👍 I will add an (optional)
to the label and let the user save the field without a script.
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.
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.
sorry for being late, maybe users will wonder what the runtime field will do when a script is not defined, it may be worth to include that in the UI
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 catch Luca. I created a PR to add text that explains this behavior: #85204
<EuiSpacer size="l" /> | ||
|
||
{/* Script */} | ||
<UseField<string> path="script"> | ||
<UseField<string> path="script.source"> |
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.
This is a comment about links.painlessSyntax
on line 229, which just goes to the main page for the Painless guide. I don't think that information is particularly useful in this context. Instead, we could link to the section for Mapping a runtime field, which provides users information about writing a Painless script in the context of a runtime field.
I believe the resulting URL would be: https://www.elastic.co/guide/en/elasticsearch/reference/master/runtime.html#runtime-mapping-fields
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.
This is great idea! I will change the link 👍
x-pack/plugins/runtime_fields/public/components/runtime_field_form/runtime_field_form.tsx
Outdated
Show resolved
Hide resolved
…mappings_editor/__jest__/client_integration/runtime_fields.test.tsx Co-authored-by: Adam Locke <adam.locke@elastic.co>
Co-authored-by: Adam Locke <adam.locke@elastic.co>
Thanks for the review @lockewritesdocs ! I've committed your change suggestions and made the suggested change. Thanks for pointing out that the script is now optional! 👍 Can you have another look? Cheers! |
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.
👏 Looks great @sebelga! I built locally and tested. Thanks for integrating these changes.
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
This PR integrates the runtime field editor (from the
runtimeFields
x-pack plugin) into the mappings editor (index_management
x-pack plugin). As the mappings editor is integrated into composable index template, legacy index template, and component template wizard, theruntime
section of the mappings is automatically supported in those three resources.Note: I removed the release note from #79940 and added it here.
Notes for code reviewers
How to test
Note: There should be a "Learn more" link. I asked for the link to the docs team and pointed it correctly. It does not work yet as the docs are not deployed yet. cc @DocTeam
emit("test")
Shadowing mapped field
Prevent duplicated runtime field
Handling ES error
As part of this PR I fixed #77165 which was required to help the user better understand possible errors in the runtime script when saving the template. For that, I created an
es_error_parser.ts
reusable handler in our "es_ui_shared/server" folder.Tests
I added all the necessary component integration tests for the support of the
runtime
tab in the mappings. I also updated the API integration tests to verify that thees_error_parser
correctly wraps the returned error from our endpoint.Copy review
If a runtime field has the same name as a mapped field, a "shadowed" badge appears with a tooltip.
In the runtime field editor, a callout indicates that this field is shadowing a mapped field.
When there are 1 or more fields, there is a tagline on top with a "Learn more" link.
Release note
Index templates and component templates can now be configured with runtime fields in their mappings.
Fixes #77165