[Fleet] Create a new `UIExtensionPoint` to replace the integration define step #149653

orouz · 2023-01-27T01:16:20Z

Summary

adds a new extension point to replace a package installation first step entirely.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

orouz · 2023-01-29T16:04:05Z

x-pack/plugins/fleet/public/types/ui_extensions.ts

+export type PackagePolicyReplaceDefineStepExtensionComponentProps = (
+  | (PackagePolicyEditExtensionComponentProps & { isEditPage: true })
+  | (PackagePolicyCreateExtensionComponentProps & { isEditPage: false })
+) & {
+  validationResults?: PackagePolicyValidationResults;
+  agentPolicy?: AgentPolicy;
+  packageInfo: PackageInfo;
+  integrationInfo?: RegistryPolicyTemplate;
+};


since this extension replaces the first step entirely, and it slightly differs between edit and create, i've used the same props from the relevant existing extension points.

the added props are from the actual props StepDefinePackagePolicy receives, so any functionality it does inside an be replicated.

orouz · 2023-01-29T16:04:11Z

...ications/fleet/sections/agent_policy/create_package_policy_page/single_page_layout/index.tsx

+  if (replaceDefineStepView && extensionView) {
+    // eslint-disable-next-line no-console
+    console.warn(
+      "'package-policy-create' is ignored when 'package-policy-replace-define-step' is defined"
+    );
+  }


not sure logging here is good enough, we could add useUIExtensions which would be typed to not allow this combination

Yeah I think useUIExtensions or adding an overload to useUIExtension that takes an array of extensions would be the path forward here. Agree that logging is not enough to prevent this, as we should prevent it entirely at compile time.

@kpollich

thought about this again and i believe we shouldn't overload useUIExtension, because:

registration is not really constrained, so we could narrow the types but that won't make them true

we can't fully limit registration with static types, as it can be done multiple times.

we can make registerExtension take an array and type-check against invalid combinations, and run a full sweep on other plugins, which would somewhat mitigate possible future risk of misuse

there is actually not much to do with that information. we still need to account for both cases, and when doing those we don't need the other extension anyway. after making the types work, i noticed the distinction wasn't really used

overloading wasn't pretty

let me know what you think is best here/follow ups:

add/iterate on overloading commit

just drop/keep the comments

work on registerExtension

something else?

thanks!

also note i'd still want to merge #149137 as soon as we can because there are other PRs depending on it

Following up on our chat offline:

Let's just throw an error when we encounter this case. This should result in the Fleet UI's error boundary being displayed and should catch the "incompatible UI extension" state during development. I don't want to silently ignore a UI extension with a browser console log/warning because it will be non-obvious to someone developing in this situation.

if (replaceDefineStepView && extensionView) { throw new Error( "'package-policy-create' and 'package-policy-replace-define-step' cannot both be registered as UI extensions" ) }

Thanks for investigating here and following up.

added this with some test updates that were needed

kpollich

No real issues for me on this - think things look pretty standard alongside the other UI extensions. The only wrinkle is the exclusivity between package-policy-create and package-policy-replace-define-step which I think we should solve via TypeScript assertions as you've suggested.

Once that's addressed I'd be more than happy to +1 this and ship. Awesome work! 🚀

kpollich · 2023-01-30T20:45:18Z

...ications/fleet/sections/agent_policy/create_package_policy_page/single_page_layout/index.tsx

+  if (replaceDefineStepView && extensionView) {
+    // eslint-disable-next-line no-console
+    console.warn(
+      "'package-policy-create' is ignored when 'package-policy-replace-define-step' is defined"
+    );
+  }


Yeah I think useUIExtensions or adding an overload to useUIExtension that takes an array of extensions would be the path forward here. Agree that logging is not enough to prevent this, as we should prevent it entirely at compile time.

elasticmachine · 2023-01-30T20:47:46Z

Pinging @elastic/fleet (Team:Fleet)

kpollich · 2023-02-01T14:01:09Z

...ications/fleet/sections/agent_policy/create_package_policy_page/single_page_layout/index.tsx

+  if (replaceDefineStepView && extensionView) {
+    // eslint-disable-next-line no-console
+    console.warn(
+      "'package-policy-create' is ignored when 'package-policy-replace-define-step' is defined"
+    );
+  }


Following up on our chat offline:

Let's just throw an error when we encounter this case. This should result in the Fleet UI's error boundary being displayed and should catch the "incompatible UI extension" state during development. I don't want to silently ignore a UI extension with a browser console log/warning because it will be non-obvious to someone developing in this situation.

if (replaceDefineStepView && extensionView) { throw new Error( "'package-policy-create' and 'package-policy-replace-define-step' cannot both be registered as UI extensions" ) }

Thanks for investigating here and following up.

kibana-ci · 2023-02-01T16:23:09Z

💚 Build Succeeded

Buildkite Build
Commit: 5e0cec2

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
`fleet`	923.5KB	924.4KB	+926.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`fleet`	25	26	+1

History

💛 Build #103700 was flaky 4aad0dd
💔 Build #103671 failed 48c18ac
💚 Build #103583 succeeded 2499179
💚 Build #103223 succeeded 7ca8dede356c0b7f8509ad600e33578ca95c0c84

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

…fine step (elastic#149653)

orouz force-pushed the fleet_replace_step_ext branch from 7ca8ded to 105a50e Compare January 29, 2023 13:03

add new extension

2499179

orouz force-pushed the fleet_replace_step_ext branch from 105a50e to 2499179 Compare January 29, 2023 13:07

orouz commented Jan 29, 2023

View reviewed changes

orouz mentioned this pull request Jan 29, 2023

[Cloud Posture] Use extension point to replace integration configuration #149137

Merged

orouz added 2 commits January 30, 2023 12:08

simplify rendering of new extension point

48c18ac

remove integration info prop

4aad0dd

orouz marked this pull request as ready for review January 30, 2023 14:53

orouz requested a review from a team as a code owner January 30, 2023 14:53

orouz requested review from kpollich and nchaulet January 30, 2023 14:53

orouz added v8.7.0 release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Jan 30, 2023

kpollich reviewed Jan 30, 2023

View reviewed changes

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jan 30, 2023

kfirpeled linked an issue Jan 31, 2023 that may be closed by this pull request

[Cloud Posture] Hide default integration configuration component #147733

Closed

4 tasks

kpollich approved these changes Feb 1, 2023

View reviewed changes

orouz added 5 commits February 1, 2023 16:59

throw on invalid extension combo

7af3d1d

add and fix tests

e304afe

Merge branch 'main' into fleet_replace_step_ext

d63154a

clean up

5d0274e

clean up

5e0cec2

orouz merged commit f002889 into elastic:main Feb 1, 2023

kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023

[Fleet] Create a new UIExtensionPoint to replace the integration de…

93d6c63

…fine step (elastic#149653)

orouz mentioned this pull request Feb 22, 2023

[Fleet] Render extension replacement component after package policy init #151876

Merged

orouz mentioned this pull request Feb 22, 2023

[Fleet] Update package policy id when agent id changes #151886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] Create a new `UIExtensionPoint` to replace the integration define step #149653

[Fleet] Create a new `UIExtensionPoint` to replace the integration define step #149653

orouz commented Jan 27, 2023 •

edited

Loading

orouz Jan 29, 2023 •

edited

Loading

orouz Jan 29, 2023 •

edited

Loading

kpollich Jan 30, 2023

orouz Jan 31, 2023

kpollich Feb 1, 2023

orouz Feb 1, 2023

kpollich left a comment

kpollich Jan 30, 2023

elasticmachine commented Jan 30, 2023

kpollich Feb 1, 2023

kibana-ci commented Feb 1, 2023

[Fleet] Create a new UIExtensionPoint to replace the integration define step #149653

[Fleet] Create a new UIExtensionPoint to replace the integration define step #149653

Conversation

orouz commented Jan 27, 2023 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

orouz Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

orouz Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

kpollich Jan 30, 2023

Choose a reason for hiding this comment

orouz Jan 31, 2023

Choose a reason for hiding this comment

kpollich Feb 1, 2023

Choose a reason for hiding this comment

orouz Feb 1, 2023

Choose a reason for hiding this comment

kpollich left a comment

Choose a reason for hiding this comment

kpollich Jan 30, 2023

Choose a reason for hiding this comment

elasticmachine commented Jan 30, 2023

kpollich Feb 1, 2023

Choose a reason for hiding this comment

kibana-ci commented Feb 1, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Public APIs missing exports

History

[Fleet] Create a new `UIExtensionPoint` to replace the integration define step #149653

[Fleet] Create a new `UIExtensionPoint` to replace the integration define step #149653

orouz commented Jan 27, 2023 •

edited

Loading

orouz Jan 29, 2023 •

edited

Loading

orouz Jan 29, 2023 •

edited

Loading