-
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
script field support, fix field formatters #33910
script field support, fix field formatters #33910
Conversation
Oops, meant to create this as a draft PR. Looks like you really can't do that after the PR is created. |
6abfd3f
to
3e77cf5
Compare
f43ad9f
to
34677f2
Compare
d5656e8
to
4145f0d
Compare
[key: string]: { id: string; params: { pattern: string } }; | ||
attributes: { | ||
fieldFormatMap: string; | ||
fields: string; |
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 some structure tweaks to this type made, after realizing that buildEsQuery
and getFieldFormatMap
both require an index pattern object, but expect different structure :\
@@ -47,3 +46,5 @@ export default async function ({ readConfigFile }) { | |||
esTestCluster: xPackFunctionalTestsConfig.get('esTestCluster'), | |||
}; | |||
} | |||
|
|||
export default getApiIntegrationConfig; |
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.
Just a little trick I saw in another test file to get my test config to "extend" this one in an easier way.
💚 Build Succeeded |
@@ -6,6 +6,7 @@ | |||
|
|||
import _ from 'lodash'; | |||
|
|||
// FIXME shouldn't this really use a service provided by @kbn-es-query? The logic here can part from Discover |
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.
Nitpick: The logic here can part from Discover
, for some reason I have a hard time reading this. Does this mean that the logic here can be separated from Discover? IE right now it's closely coupled to it?
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'll fix the comment, but this is meaning that the logic can part ways with Discover, even though they do much of the same thing. That becomes the cause of bugs like #25068
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.
Small nitpicks, otherwise looks good
@@ -132,6 +147,19 @@ export async function generateCsvSearch( | |||
const esQueryConfig = await getEsQueryConfig(uiConfig); | |||
const [sortField, sortOrder] = savedSearchObjectAttr.sort; | |||
|
|||
const scriptFields = indexPatternSavedObject.fields | |||
.filter((f: IndexPatternField) => f.scripted === true) |
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.
.filter((f: IndexPatternField) => f.scripted)
@@ -42,19 +42,16 @@ export async function getDataSource( | |||
} | |||
} | |||
try { |
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.
Nice cleanup here.
…/reporting/csv-export-panel-action-script-fields-for-pr
💚 Build Succeeded |
LGTM! |
This adds support for CSV data columns that are represented by scripted fields in Kibana.
BEFORE:
AFTER:
It also leverages field formatters, which previously were in the code but not working.