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

[Discover] Fix Discover navigation from Lens embeddable #147000

Merged

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Dec 5, 2022

Summary

Fixes #146761

This PR fixes navigation to Discover from Lens embeddable.

Checklist

@dimaanj dimaanj added Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.7.0 v8.6.1 labels Dec 5, 2022
@dimaanj dimaanj self-assigned this Dec 5, 2022
…cover-navigation-from-lens-embeddable

# Conflicts:
#	x-pack/plugins/lens/public/trigger_actions/open_in_discover_action.test.ts
@dimaanj dimaanj requested a review from a team December 6, 2022 13:21
@dimaanj dimaanj marked this pull request as ready for review December 6, 2022 13:21
@dimaanj dimaanj requested a review from a team as a code owner December 6, 2022 13:21
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Pulled and tested locally, LGTM. Thanks for fixing! We should address @dej611's concern before merging though.

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

I think more code can still be removed.
But why is it required to return the spec rather than the id? What additional fields are required to make the navigation work? I cannot see it from this PR code.

dimaanj and others added 4 commits December 8, 2022 16:10
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 8, 2022

But why is it required to return the spec rather than the id? What additional fields are required to make the navigation work? I cannot see it from this PR code.

We should provide spec for Discover navigation, since it's adhoc data view, which is not saved.

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 8, 2022

@elasticmachine merge upstream

@dimaanj dimaanj requested a review from dej611 December 8, 2022 14:36
@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 14, 2022

@elasticmachine merge upstream

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested locally with Safari 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.3MB 1.3MB +94.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
unifiedSearch 50.5KB 50.6KB +46.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 60 66 +6
osquery 109 115 +6
securitySolution 445 451 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 69 75 +6
osquery 110 117 +7
securitySolution 521 527 +6
total +21

History

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

cc @dimaanj

@dimaanj dimaanj added the auto-backport Deprecated - use backport:version if exact versions are needed label Dec 14, 2022
@dimaanj dimaanj merged commit 1f04bf8 into elastic:main Dec 14, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.6 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 147000

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 14, 2022
* main: (21 commits)
  [Profiling] Remove link to 'Other' bucket (elastic#147523)
  [Synthetics UI] Add missing configuration options to the add/edit monitor forms (elastic#147265)
  [DOCS] Updates what's new pages (elastic#147483)
  [Fleet][Endpoint][RBAC V2] Update fleet router and config to allow API access via RBAC controls (elastic#145361)
  [Guided onboarding] Update guide IDs (elastic#147348)
  [Synthetics] Add synthetics settings alerting default (elastic#147339)
  [Security Solution][Endpoint] Fix Policy form being displayed as Read Only when displayed in Fleet pages (elastic#147212)
  [Cases] Save draft user comment (elastic#146327)
  [API Docs] Fix `--plugin` filter (elastic#147500)
  [Fleet] added a logic to use `destinationId` when tagging imported SOs (elastic#147439)
  Do not skip UPDATE_TARGET_MAPPINGS if upgrading to a newer stack version (elastic#147503)
  [Discover] Validate if Data View time field exists on Alert creation / editing (elastic#146324)
  [Discover] Fix Discover navigation from Lens embeddable (elastic#147000)
  Allow users to Update API Keys (elastic#146237)
  Update dependency xstate to ^4.35.0 (main) (elastic#147463)
  [Behavioral Analytics] Remove feature flag to hide functionality (elastic#147429)
  [Fleet] Add agent policy `inactivity_timeout`experimental setting (elastic#147432)
  [APM] Switching service groups from grid to flex layout (elastic#147448)
  [Fleet] Add missing endpoints to openApi specs (elastic#147452)
  [AO] Allow providing custom time range for Alert Summary Widget (elastic#147253)
  ...
@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 15, 2022

💚 All backports created successfully

Status Branch Result
8.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

dimaanj added a commit to dimaanj/kibana that referenced this pull request Dec 15, 2022
## Summary

Fixes elastic#146761

This PR fixes navigation to Discover from Lens embeddable.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
(cherry picked from commit 1f04bf8)

# Conflicts:
#	x-pack/plugins/lens/public/trigger_actions/open_in_discover_action.test.ts
dimaanj added a commit that referenced this pull request Dec 15, 2022
… (#147608)

# Backport

This will backport the following commits from `main` to `8.6`:
- [[Discover] Fix Discover navigation from Lens embeddable
(#147000)](#147000)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Dmitry
Tomashevich","email":"39378793+dimaanj@users.noreply.github.com"},"sourceCommit":{"committedDate":"2022-12-14T13:17:17Z","message":"[Discover]
Fix Discover navigation from Lens embeddable (#147000)\n\n##
Summary\r\n\r\nFixes #146761\r\n\r\nThis PR fixes navigation to Discover
from Lens embeddable.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\nCo-authored-by: Marco Liberati
<dej611@users.noreply.github.com>","sha":"1f04bf89a3df377c79dba225dd47c9a96dc3e395","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:fix","auto-backport","Team:DataDiscovery","v8.7.0","v8.6.1"],"number":147000,"url":"https://github.com/elastic/kibana/pull/147000","mergeCommit":{"message":"[Discover]
Fix Discover navigation from Lens embeddable (#147000)\n\n##
Summary\r\n\r\nFixes #146761\r\n\r\nThis PR fixes navigation to Discover
from Lens embeddable.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\nCo-authored-by: Marco Liberati
<dej611@users.noreply.github.com>","sha":"1f04bf89a3df377c79dba225dd47c9a96dc3e395"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/147000","number":147000,"mergeCommit":{"message":"[Discover]
Fix Discover navigation from Lens embeddable (#147000)\n\n##
Summary\r\n\r\nFixes #146761\r\n\r\nThis PR fixes navigation to Discover
from Lens embeddable.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\nCo-authored-by: Marco Liberati
<dej611@users.noreply.github.com>","sha":"1f04bf89a3df377c79dba225dd47c9a96dc3e395"}},{"branch":"8.6","label":"v8.6.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
nreese pushed a commit to nreese/kibana that referenced this pull request Dec 16, 2022
## Summary

Fixes elastic#146761

This PR fixes navigation to Discover from Lens embeddable.


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.6.0 v8.6.1 v8.7.0
Projects
None yet
6 participants