-
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
[ES|QL] Do not allow saving in library action on text based panels #167111
Conversation
Pinging @elastic/kibana-presentation (Team:Presentation) |
Pinging @elastic/kibana-visualizations (Team:Visualizations) |
const { isOfAggregateQueryType } = await import('@kbn/es-query'); | ||
const query = isFilterableEmbeddable(embeddable) && (await embeddable.getQuery()); | ||
// Textbased panels (i.e. ES|QL, SQL) should not save to library | ||
const isTextBasedEmbeddable = isOfAggregateQueryType(query as AggregateQuery); |
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.
Could you add a test to packages/kbn-es-query/src/es_query/es_aggregate_query.test.ts that verifies isOfAggregateQueryType returns false when query is undefined?
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.
Done d1df81f
|
||
const { isOfAggregateQueryType } = await import('@kbn/es-query'); | ||
const query = isFilterableEmbeddable(embeddable) && (await embeddable.getQuery()); | ||
// Textbased panels (i.e. ES|QL, SQL) should not save to library |
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.
Will this always be true? Why can't text based panels be saved to library? Would it be better to put an explicit flag on embeddable interface?
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 business decision we made for ES|QL. We don' t want them to be saved in library or being edited in Lens. The visualizations created with ESQL will always be by value ones.
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.
kibana-presentation changes LGTM
code review only
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.
Data Discovery changes LGTM 👍
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
Save to library action should not be present in ES|QL panels. For now we only allow by value embeddables and we don't want them to be edited/created in Lens editor
Checklist