-
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
Adds @kbn/utils package #76518
Adds @kbn/utils package #76518
Conversation
8e731a9
to
1d1b0e8
Compare
2874cb6
to
cbdacce
Compare
Moves common utility functions to obtain the repository root, paths (config/data), and Kibana package.json to a @kbn/utils package. Moving these existing functions allows them to be used in production, in other packages because Kibana. Signed-off-by: Tyler Smalley <[email protected]>
cbdacce
to
ea40aab
Compare
Pinging @elastic/kibana-operations (Team:Operations) |
Pinging @elastic/kibana-platform (Team:Platform) |
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.
Security team changes LGTM
x-pack/test/spaces_api_integration/common/config.ts
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.
Telemetry changes LGTM!
Co-authored-by: Thomas Watson <[email protected]>
Signed-off-by: Tyler Smalley <[email protected]>
@@ -21,7 +21,7 @@ import { existsSync } from 'fs'; | |||
import { join } from 'path'; | |||
|
|||
import { Logger } from '../cli_plugin/lib/logger'; | |||
import { getConfigDirectory, getDataPath } from '../core/server/path'; | |||
import { getConfigDirectory, getDataPath } from '@kbn/utils'; |
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.
Is it okay that getDataPath
ignores a value configured in kibana.yml
? We've even got an issue for this #43539
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.
The plan is to first move this over, then resolve that issue - might wait for #76874 to assist with that.
@@ -0,0 +1,3 @@ | |||
# @kbn/utils | |||
|
|||
Shared server-side utilities shared across packages and plugins. |
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.
btw are we sure that we'd never need Kibana-specific browser utils?
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's possible but thought it would be best to keep it focused to start.
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 expect we will quickly start hearing stories of @kbn/utils
gets pulled into the browser builds accidentally and failing because things like REPO_ROOT
don't work in the browser (and will throw an error on page load, maybe even at build time).
It seems like we'll need to have separate entry points, like @kbn/utils/node
and @kbn/utils/browser
, which have slightly different APIs but share the exports they can. Still, something we can do in a follow-up as needed.
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.
LGTM for stack monitoring, as long as CI is passing!
Signed-off-by: Tyler Smalley <[email protected]>
Signed-off-by: Tyler Smalley <[email protected]>
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.
Canvas changes lgtm
@@ -128,6 +128,7 @@ | |||
/packages/kbn-test/ @elastic/kibana-operations | |||
/packages/kbn-ui-shared-deps/ @elastic/kibana-operations | |||
/packages/kbn-es-archiver/ @elastic/kibana-operations | |||
/packages/kbn-utils/ @elastic/kibana-operations |
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.
don't forget to remove this before back-porting.
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.
ok for the platform changes
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.
LGTM, though I don't love jest-when
and wonder if it might actually be more reasonable to start with a simple helper defined in x-pack/plugins/canvas/server/routes/shareables/download.test.ts
rather than pulling in a package which bundles bunyan
.
💚 Build SucceededBuild metricsasync chunks size
page load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
Moves common utility functions to obtain the repository root, paths (config/data), and Kibana package.json to a @kbn/utils package. Moving these existing functions allows them to be used in production, in other packages because of Kibana. Signed-off-by: Tyler Smalley <[email protected]>
Moves common utility functions to obtain the repository root, paths (config/data), and Kibana package.json to a @kbn/utils package. Moving these existing functions allows them to be used in production, in other packages because of Kibana. Signed-off-by: Tyler Smalley <[email protected]>
Moves common utility functions to obtain the repository root, paths (config/data), and Kibana package.json to a @kbn/utils package. Moving these existing functions allows them to be used in production, in other packages because of Kibana. Signed-off-by: Tyler Smalley <[email protected]> * [@kbn/utils] Adds missing dependency (#77536) Signed-off-by: Tyler Smalley <[email protected]>
…s-for-710 * 'master' of github.com:elastic/kibana: (95 commits) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) [@kbn/utils] Adds missing dependency (elastic#77536) Add the Enterprise Search logo to the Overview search result (elastic#77514) [Logs UI] [Metrics UI] Remove saved object field mappings (elastic#75482) [Security Solution][Exceptions] Exception modal bulk close option only closes alerts generated by same rule (elastic#77402) Adds @kbn/utils package (elastic#76518) Map layout changes (elastic#77132) [Security Solution] [Detections] EQL Rule Creation (elastic#76831) Adding test user to maps tests under documents source folder (elastic#77245) Suppress error logs when clients connect over HTTP instead of HTTPS (elastic#77397) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx
This moves some common utility functions to obtain the repository root, paths (config/data), and Kibana
package.json
to a@kbn/utils
package. Moving these existing functions allows them to be used in production across plugins and packages.Related to #76874
Blocks #75521