-
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
[Fleet][EPM] Add TS type for package-spec. Clarify EPR relationship with spec #84946
[Fleet][EPM] Add TS type for package-spec. Clarify EPR relationship with spec #84946
Conversation
Clarify EPR types relationship with package spec. i.e. what required properties it makes optional, what properties it adds, which property types it changes, etc. 3 TS errors and I think they're all expected to be fixed by #84944 ERROR x-pack failed x-pack/plugins/fleet/public/applications/fleet/hooks/use_package_icon_type.ts:45:66 - error TS2339: Property 'path' does not exist on type 'PackageSpecIcon'. 45 const localIconSrc = Array.isArray(svgIcons) && (svgIcons[0].path || svgIcons[0].src); ~~~~ x-pack/plugins/fleet/public/applications/fleet/sections/epm/screens/detail/overview_panel.tsx:13:24 - error TS2339: Property 'readme' does not exist on type 'PackageInfo'. 13 const { screenshots, readme, name, version } = props; ~~~~~~ x-pack/plugins/fleet/public/applications/fleet/sections/epm/screens/detail/overview_panel.tsx:18:36 - error TS2322: Type 'PackageSpecScreenshot[] | (PackageSpecScreenshot[] & RegistryImage[])' is not assignable to type 'RegistryImage[]'. Type 'PackageSpecScreenshot[]' is not assignable to type 'RegistryImage[]'. Property 'path' is missing in type 'PackageSpecScreenshot' but required in type 'RegistryImage'. 18 {screenshots && <Screenshots images={screenshots} />} ~~~~~~ x-pack/plugins/fleet/common/types/models/epm.ts:115:3 115 path: string; ~~~~ 'path' is declared here. x-pack/plugins/fleet/public/applications/fleet/sections/epm/screens/detail/screenshots.tsx:14:3 14 images: ScreenshotItem[]; ~~~~~~ The expected type comes from property 'images' which is declared here on type 'IntrinsicAttributes & ScreenshotProps' Found 3 errors.
Thanks for cleaning up and organizing the types! 🎉 Looks good once CI passes. |
@elasticmachine merge upstream |
merge conflict between base and head |
title: string; | ||
latestVersion: string; | ||
assets: AssetsGroupedByServiceByType; | ||
removable?: boolean; | ||
} | ||
|
||
type Merge<FirstType, SecondType> = Omit<FirstType, Extract<keyof FirstType, keyof SecondType>> & |
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.
From https://github.com/sindresorhus/type-fest/#utilities. Can move elsewhere later if needed.
> | ||
> | ||
>; | ||
response: Array<Installable<RegistrySearchResult>>; |
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.
Reference existing type instead of duplicating the definition
@@ -29,8 +29,7 @@ export const AssetTitleMap: Record<AssetType, string> = { | |||
map: 'Map', | |||
}; | |||
|
|||
export const ServiceTitleMap: Record<ServiceName, string> = { | |||
elasticsearch: 'Elasticsearch', | |||
export const ServiceTitleMap: Record<Extract<ServiceName, 'kibana'>, string> = { |
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.
Package spec (currently) limits values to kibana
): string | undefined => { | ||
const sourcePath = img.src | ||
? `/package/${pkgName}/${pkgVersion}${img.src}` | ||
: 'path' in img && img.path; |
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.
path
doesn't exist in PackageSpec*
@@ -69,18 +70,20 @@ export function Screenshots(props: ScreenshotProps) { | |||
<EuiSpacer /> | |||
</NestedEuiFlexItem> | |||
)} | |||
<NestedEuiFlexItem> | |||
{/* By default EuiImage sets width to 100% and Figure to 22.5rem for size=l images, | |||
{screenshotUrl && ( |
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.
make sure we have an image URL before attempting to render it.
Pinging @elastic/ingest-management (Team:Ingest Management) |
@@ -29,7 +28,11 @@ describe('Fleet - packageToPackagePolicy', () => { | |||
map: [], | |||
}, | |||
}, | |||
status: installationStatuses.NotInstalled, |
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.
Not strictly required for this change. Only because I was in here and it's still type safe but w/one less import
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@@ -1310,6 +1310,7 @@ export class EndpointDocGenerator { | |||
}, | |||
], | |||
status: 'installed', | |||
release: 'ga', |
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.
release
key is required by package-spec and can be one of experimental
, beta
, or ga
@@ -1298,7 +1298,7 @@ export class EndpointDocGenerator { | |||
title: 'Elastic Endpoint', | |||
version: '0.5.0', | |||
description: 'This is the Elastic Endpoint package.', | |||
type: 'solution', | |||
type: 'integration', |
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.
integration
is the only valid type
value
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 @jfsiii
Thanks for the changes to the Endpoint generator
…ith spec (#84946) (#85139) ## Summary * Create a [TS type for `package-spec`](https://github.com/elastic/kibana/blob/09cce235de55f898043e969a234464e62473cba9/x-pack/plugins/fleet/common/types/models/package_spec.ts) to differentiate Registry shape from spec shape * Clarify EPR types relationship with package spec. i.e. what required properties it makes optional, what properties it adds, which property types it changes, etc. https://github.com/elastic/kibana/blob/5f2c4a5547fc9a70b2696ca34c7caff64f3ed674/x-pack/plugins/fleet/common/types/models/epm.ts#L71-L80 * Updated `manifest.yml` in `apache_invalid_manifest_missing_field_0.1.4.zip`. Previously it was missing the `type` property, but that's optional [according to package-spec](https://github.com/elastic/package-spec/blob/3b4d8207554dde6ab9646567dd39b747a2ce167c/versions/1/manifest.spec.yml#L241-L248). Removed the required `format_version` and re-zipped. <details><summary>screenshots show errors if catches in a mock PackageInfo object</summary> using `requirement`; not `conditions` <img width="1084" alt="Screen Shot 2020-12-02 at 12 07 15 PM" src="https://user-images.githubusercontent.com/57655/101083986-4b915f80-357b-11eb-84c6-14605e1ea3d8.png"> using `versions`; not `version` <img width="1395" alt="Screen Shot 2020-12-02 at 12 08 13 PM" src="https://user-images.githubusercontent.com/57655/101083988-4c29f600-357b-11eb-9ff2-2b73e313c130.png"> including `elasticsearch` when spec says it' not valid yet <img width="1589" alt="Screen Shot 2020-12-02 at 12 08 36 PM" src="https://user-images.githubusercontent.com/57655/101083990-4c29f600-357b-11eb-8250-8926f7189af8.png"> </details> <details><summary>screenshots showing editor autocomplete for registry response values like <code>categories</code></summary> <img width="422" alt="Screen Shot 2020-12-02 at 12 25 11 PM" src="https://user-images.githubusercontent.com/57655/101083994-4cc28c80-357b-11eb-9013-ae208f7c311a.png"> <img width="345" alt="Screen Shot 2020-12-02 at 12 25 01 PM" src="https://user-images.githubusercontent.com/57655/101083991-4c29f600-357b-11eb-9468-bbb9d27513b1.png"> </details> <details><summary>screenshot showing type check preventing adding properties which aren't explicitly listed as "additions"</summary> <img width="1295" alt="Screen Shot 2020-12-03 at 11 38 43 AM" src="https://user-images.githubusercontent.com/57655/101083997-4cc28c80-357b-11eb-83b9-206b85521f11.png"> </details> ### Checklist - [ ] [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
💔 Build Failed
Failed CI StepsTest FailuresX-Pack Jest Tests.x-pack/plugins/index_management/public/application/components/mappings_editor/__jest__/client_integration/datatypes.Mappings editor: scaled float datatype should require a scaling factor to be providedStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
package-spec
to differentiate Registry shape from spec shapekibana/x-pack/plugins/fleet/common/types/models/epm.ts
Lines 71 to 80 in 5f2c4a5
manifest.yml
inapache_invalid_manifest_missing_field_0.1.4.zip
. Previously it was missing thetype
property, but that's optional according to package-spec. Removed the requiredformat_version
and re-zipped.screenshots show errors if catches in a mock PackageInfo object
using `requirement`; not `conditions` using `versions`; not `version` including `elasticsearch` when spec says it' not valid yetscreenshots showing editor autocomplete for registry response values like
categories
screenshot showing type check preventing adding properties which aren't explicitly listed as "additions"
Checklist