[EPM] Allow Canvas Workpad Template asset type to Integrations #71267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crob611
added
review
loe:small
botelastic
bot
added
the
Team:Fleet
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
skh
reviewed
Jul 13, 2020
| @@ -29,6 +29,7 @@ export enum KibanaAssetType { | |||
| search = 'search', | |||
| indexPattern = 'index-pattern', | |||
| map = 'map', | |||
| 'canvas-workpad-template' = 'canvas-workpad-template', | |||
There was a problem hiding this comment.
Can we have this camel-cased here, so canvasWorkpadTemplate, following the indexPattern two lines above?
There was a problem hiding this comment.
@skh I think there might be a bug here that prevents this. (But I might be misunderstanding how a package should be structured).
If the file (in this case) is package-version/kibana/canvas-workpad-template/whatever.json then the type will be canvas-workpad-template and the in KibanaAssetType check here would fail if KibanaAssetType had the camel cased key.
So I guess the question is should the types in the package match what's defined in this KibanaAssetType or match the actual Kibana Saved Object Type?
I could not find any example packages that actually had an "index-pattern" or "indexPattern" folder, but the examples in the package-registry repo uses "index-pattern" for the folder name, which would fail to have those assets show up.
There was a problem hiding this comment.
I'm afraid you're right. Thanks for spotting this, I need to look into it.
|
This looks like it should work, but I would feel better if we had at least a package to test manually with, or an integration test added to |
|
@skh I added a unit test for the part of the registry that makes use of the KibanaAssetType enum to ensure it's only adding the known asset types. I can also add an integration test if you don't think this unit test covers what you're looking for. |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/lens/smokescreen·ts.lens app lens smokescreen tests should allow creation of lens visualizationsStandard OutStack TraceBuild metrics
History
To update your PR or re-run it, just comment with: |
|
Thanks for the unit test! Eventually I'd love to see an integration test that performs an actual package installation, but that can be added in a follow-up PR -- I need one for the I'd like to do some smoke testing on this locally, will approve when I'm done. |
|
However, this PR will probably bite you anyway: elastic/package-registry#572 (forbids |
|
@crob611 Should this PR remain open? |
|
Closing for now. Will open a similar PR once #71633 is resolved. |
|
@crob611 great to see progress on the canvas part! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Adds Canvas Workpad Templates as an allowable type in packages. We might be putting out some of these templates in a package soon, so want to make sure it's available in 7.9.