-
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
[Asset Manager] Creates baseline public asset client for use in public plugins #167191
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
6dd1ac9
to
64602d7
Compare
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.
limits.yml
The "enabled" tests are NOT run by CI yet, to prevent blocking Kibana development for a | ||
test failure in this alpha, tech preview plugin. They will be moved into the right place | ||
to make them run for CI before the plugin is enabled by default. To run them manually: | ||
|
||
```shell | ||
$ node scripts/functional_tests_server --config x-pack/test/api_integration/apis/asset_manager/config.ts | ||
$ node scripts/functional_test_runner --config=x-pack/test/api_integration/apis/asset_manager/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.
The original config.ts
test was split in two configurations, one for testing apis with assets source and the other for signals. Currently the signals test suite is enabled
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.
Oh ok good call out, I'll take a look and maybe adjust slightly in my next PR
x-pack/plugins/asset_manager/public/lib/public_assets_client.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/asset_manager/public/lib/public_assets_client.test.ts
Outdated
Show resolved
Hide resolved
any Kibana plugin without any dependency restrictions. To gain access to the asset data, | ||
this component or tool can require the appropriate asset client to be passed in. | ||
|
||
TODO: need to move main client types to a package so that they can be depended on by |
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.
can the packages import types from assetmanager plugin instead ?
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.
Should packages import from plugins? That sounds wrong, but maybe it's allowed for types only? Not sure it's a good idea...
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.
packages already do that (eg kbn-data-service, alert_details..) but I don't know if that's recommended
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, I'll try to poke around and think on that a bit.
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
This recently merged PR is going to cause conflicts for this PR -- I'll fix. |
759b999
to
2738219
Compare
I think there is an issue with public access to the config. I can't seem to enable the public plugin because my config isn't being read. |
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.
Change to test/plugin_functional/test_suites/core_plugins/rendering.ts
LGTM
@kibanamachine run elasticsearch-ci/docs |
1 similar comment
@kibanamachine run elasticsearch-ci/docs |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
Closes #167075
Summary
Adds a public asset client available in the
setup
lifecycle hook for plugins that depend on this one.getHosts
is the only method available on this client for now.TODO, before merge:
Testing this PR
One way of testing this new client is to apply the attached test-assets.patch file locally, adjust the date range in the getHosts query that is added in the infra plugin, and then start Kibana and navigate to the infra app. You should see print out in the browser console.
test-assets.patch