[ILM] Migrate Hot phase to Form Lib #80012

jloleysens · 2020-10-08T15:22:12Z

Summary

Migrate the hot phase in the ILM form to use the form lib.?

Duplicated the SetPriority and Forcemerge components for now to create the form lib versions that will be used elsewhere
Added serialisation test coverage for hot phase to client integration tests
Removed hot phase serialisation/deserialization for hot phase from x-pack/plugins/index_lifecycle_management/public/application/services

How to review

Regression test that Index Lifecycle Management enables setting hot phase for new and existing policies
Serialisation and deserialisation code has been moved to the serialization.ts and deserialization.ts files that are used in the form that lives in edit_policy.ts.
For now, the setWarmPhaseOnRollover function is still being passed to the HotPhase component since the warm phase is not be handled by the form yet and it is used in the legacy serialization of the warm phase. This will be addressed in the next PR for this work.

New JSON policy flyout behaviour

Note: error states are not ideal, those three fields are all connected and so are correctly erroring out together. But the UX of this should probably be revisited @mdefazio

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

- tests are now broken - need to break up the hot_phase file in to meaningful parts - duplicated set_priority and forcemerge components

- moved a lot of files around - removed the need for the state to track whether rollover is set

jloleysens · 2020-10-12T09:56:05Z

@elasticmachine merge upstream

- refactor serializePolicy -> legacySerializePolicy - updated serialization of form lib to factor in pre-existing policy values. These should be interacted with in a non-lossy way.

…otphase-to-formlib * 'master' of github.com:elastic/kibana: (59 commits) [Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard (elastic#80193) [Security Solution] Reduce initial bundle size (elastic#78992) [Security Solution][Resolver] Fix Resize node box-shadow bug (elastic#80223) Move observability content (elastic#79978) skip flaky suite (elastic#79389) removing kibana_datatable` in favor of `datatable` (elastic#75184) [ML] Fixes for anomaly swim lane (elastic#80299) [Lens] Smokescreen lens test unskip (elastic#80190) Improved AlertsClient tests structure by splitting a huge alerts_client.tests.ts file into a specific files defined by its responsibility. (elastic#80088) [APM] React key warning when opening popover with external resources (elastic#80328) [Step 1] use Observables on server search API (elastic#79874) Apply back pressure in Task Manager whenever Elasticsearch responds with a 429 (elastic#75666) [Lens] Leverage original http request error (elastic#79831) [Security Solution][Case] Improve ServiceConnectorCaseParams type (elastic#80109) [SECURITY_SOLUTION] Fix query on alert histogram (elastic#80219) [DOCS] Update ingest node pipelines doc (elastic#79187) [Ingest Manager] Split up OpenAPI spec file (elastic#80107) [SECURITY_SOLUTION][ENDPOINT] Fix label on Trusted App create name field (elastic#80001) [Ingest Manager] Fix agent policy bump revision to create only one POLICY_CHANGE action (elastic#80081) Grid layout fixes (elastic#80305) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared/data_tier_allocation_field.tsx # x-pack/plugins/index_lifecycle_management/public/shared_imports.ts

jloleysens · 2020-10-14T07:26:33Z

@elasticmachine merge upstream

elasticmachine · 2020-10-14T07:26:39Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

sebelga

Awesome work @jloleysens ! Overall everything works great! I did notice one small bug: when I click the submit button without giving a name I get the validation error. Then I give the policy a name but the error is still there.

The only potential bug I've noticed is about the request JSON in the flyout, I've added a comment. We can discuss it over zoom.

I would also like to weigh in with you the benefit of having a schema for the policy form, a central place to declare the field configurations. Let's talk about it tomorrow ok? Cheers!

sebelga · 2020-10-15T12:42:30Z

src/plugins/es_ui_shared/static/forms/components/fields/numeric_field.tsx

      describedByIds={rest.idAria ? [rest.idAria] : undefined}
+      {...rest}


Great! 👍 Can we make this change to all the components?

sebelga · 2020-10-15T12:49:27Z

...s/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx

-}
+type SetupReturn = ReturnType<typeof setup>;
+
+export type EditPolicyTestBed = SetupReturn extends Promise<infer U> ? U : SetupReturn;


Nice trick! I guess we'll never hit the right part of the ternary right? But this is a way to extract U? Interesting... 😊

sebelga · 2020-10-15T13:20:33Z

...ugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts

@@ -21,6 +26,83 @@ describe('<EditPolicy />', () => {
    server.restore();
  });

+  describe('hot phase', () => {
+    describe('serialization', () => {


Awesome to have the serialization test here! 👍

sebelga · 2020-10-15T13:22:57Z

x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx

+      await act(async () => {
+        maxSizeInput.simulate('change', { target: { value: '-1' } });
+      });
+      waitForFormLibValidation();


Do you mind adding a comment explaining why in this case we need to wait for the validations? That is because there is a 300ms delay before displaying field error message?

sebelga · 2020-10-15T13:25:49Z

x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx

@@ -142,12 +139,38 @@ const setPhaseIndexPriority = (
  priorityInput.simulate('change', { target: { value: priority } });
  rendered.update();
 };
-const save = (rendered: ReactWrapper) => {
+const setPhaseIndexPriorityFormLib = async (


I think it would be better to not add the "FormLib" suffix and add a "Legacy" suffix to the other method. As we plan on migrating the rest of the test file right?

sebelga · 2020-10-15T15:19:58Z

...fecycle_management/public/application/sections/edit_policy/components/set_priority_input.tsx

@@ -3,6 +3,12 @@
 * or more contributor license agreements. Licensed under the Elastic License;
 * you may not use this file except in compliance with the Elastic License.
 */
+
+/**
+ * PLEASE NOTE: This component is currently duplicated. A version of this component wired up with


Thanks for the "Please", that's very thoughtful 😊

sebelga · 2020-10-15T15:20:39Z

...k/plugins/index_lifecycle_management/public/application/sections/edit_policy/deserializer.ts

+import { FormInternal } from './components/phases/types';
+
+export const deserializer = (policy: SerializedPolicy): FormInternal =>
+  produce<FormInternal>(


Great to see immer in action! 👍 I will have to use it too.

It is indeed very convenient! I would caution against using it on massive+nested objects. On lower memory environments it can blow the stack because it recurses :(

Good to know!

sebelga · 2020-10-15T15:26:28Z

x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/serializer.ts

+  }
+
+  if (rest.phases.hot) {
+    rest.phases.hot.min_age = originalPolicy?.phases.hot?.min_age ?? '0ms';


So the rest.phases.hot.min_age cannot be set in the form? It is simply copied over the originalPolicy or given a default value?

Yeah, it is a bit of a weird one. This is what current serialisation behaviour gives us:

kibana/x-pack/plugins/index_lifecycle_management/public/application/constants/policy.ts

Line 63 in fc5ad4d

export const serializedPhaseInitialization: SerializedPhase = {

but with a bit more indirection

sebelga · 2020-10-15T15:32:06Z

...k/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx

@@ -85,30 +127,57 @@ export const EditPolicy: React.FunctionComponent<Props> = ({
    history.push('/policies');
  };

+  const setWarmPhaseOnRollover = useCallback(


Happy to get rid of this indirection in future PR! 😊

sebelga · 2020-10-15T15:34:16Z

...k/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx

+
+              <EuiSpacer />
+
+              <HotPhase setWarmPhaseOnRollover={setWarmPhaseOnRollover} />


Nice and clean 👍

jloleysens · 2020-10-16T08:18:55Z

@elasticmachine merge upstream

…mRow

…ority

- also fix missing "key" on react component in unrelated file - fixed ordering of JSON in test file - also removed default value from form schema so that when a value is not set for max size, max docs or max age it will remain unset in future policies

jloleysens · 2020-10-19T07:55:22Z

@elasticmachine merge upstream

sebelga · 2020-10-19T09:02:37Z

x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx

@@ -165,6 +165,11 @@ describe('edit policy', () => {
    jest.useRealTimers();
  });

+  /**


sebelga · 2020-10-19T09:13:03Z

...fecycle_management/public/application/sections/edit_policy/components/policy_json_flyout.tsx

+  const form = useFormContext();
+
+  useEffect(() => {
+    const subscription = form.subscribe(async (formUpdate) => {


We will deprecate the form.subscribe in favor of the useFormData. Can we use it instead here?

Sure! I think in this particular case, the solution was less verbose with the use of form.subscribe because form.validate lives in a different context. However, I can see that useFormData should be the primary mechanism for hooking into updates for most cases.

jloleysens · 2020-10-19T10:11:51Z

@sebelga I addressed your latest round of feedback, would you mind taking another look?

kibanamachine · 2020-10-19T14:36:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 371ff40

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`indexLifecycleManagement`	208	220	+12

async chunks size

id	before	after	diff
`indexLifecycleManagement`	234.8KB	268.7KB	+33.9KB

page load bundle size

id	before	after	diff
`esUiShared`	304.3KB	304.2KB	-131.0B
`indexLifecycleManagement`	89.9KB	90.6KB	+688.0B
total			+557.0B

History

💚 Build #82387 succeeded b560f21
💚 Build #82355 succeeded 55346ba
💚 Build #82172 succeeded e5665d0
💔 Build #82147 failed 7f48a65
💚 Build #82102 succeeded a8ab7b6

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

sebelga · 2020-10-19T14:53:56Z

...fecycle_management/public/application/sections/edit_policy/components/policy_json_flyout.tsx

+   */
+  const [policy, setPolicy] = useState<undefined | null | SerializedPolicy>(undefined);
+
+  const form = useFormContext();


Minor nit, we don't need to validate on each form change if isValid is not undefined.

const form = useFormContext(); const [formData, getFormData] = useFormData(); const { isValid, validate } = form; useEffect(() => { (async function checkPolicy() { const isFormValid = isValid ?? (await validate()); if (isFormValid) { const p = getFormData() as SerializedPolicy; setPolicy({ ...legacyPolicy, phases: { ...legacyPolicy.phases, hot: p.phases.hot, }, }); } else { setPolicy(null); } })(); // eslint-disable-next-line react-hooks/exhaustive-deps }, [isValid, validate, legacyPolicy, formData]);

Also in f5edadc (#80842) I've fixed the problem that you faced when you had to add the eslint override.

Awesome, we should incorporate that into a following PR.

sebelga

Thanks for making all the changes @jloleysens! I tested locally and I'm happy with your solution regarding the validation for the request in the flyout. 👍

* wip * added missing shared_imports file to index * initial migration of hot phase to form lib - tests are now broken - need to break up the hot_phase file in to meaningful parts - duplicated set_priority and forcemerge components * Big refactor - moved a lot of files around - removed the need for the state to track whether rollover is set * Integrate form lib serialization with existing serialization - refactor serializePolicy -> legacySerializePolicy - updated serialization of form lib to factor in pre-existing policy values. These should be interacted with in a non-lossy way. * wip on fixing jest tests and some other refactors * fix jest tests and other refactors * delete existing hot phase serialization and tests * beginning of serializer test for hot phase * added serialization tests for form lib components * fix some i18n issues * fixed delete phase integration test * move hot phase serialization test to pre-existing test location * fix another jest test issue * fix ui metric tracking for setting input priority in hot phase * refactor use rollover switch to form lib component and update validation for number segments in force merge * readded missing validation 🤦🏼‍♂️ * fix type check issues and setting of rollover enabled 🙄 * migrate all form lib components to spreading all rest props in EuiFormRow * added comment to test helper function * refactor test helper setPhaseIndexPriorityFormLib -> setPhaseIndexPriority * refactor to use form schema * Removed use of UseMultiFields component - also fix missing "key" on react component in unrelated file - fixed ordering of JSON in test file - also removed default value from form schema so that when a value is not set for max size, max docs or max age it will remain unset in future policies * update json flyout behaviour * fix json policy serialization * Fix type and i18n issues * do not use form.subscribe * add missing key value in cells Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* wip * added missing shared_imports file to index * initial migration of hot phase to form lib - tests are now broken - need to break up the hot_phase file in to meaningful parts - duplicated set_priority and forcemerge components * Big refactor - moved a lot of files around - removed the need for the state to track whether rollover is set * Integrate form lib serialization with existing serialization - refactor serializePolicy -> legacySerializePolicy - updated serialization of form lib to factor in pre-existing policy values. These should be interacted with in a non-lossy way. * wip on fixing jest tests and some other refactors * fix jest tests and other refactors * delete existing hot phase serialization and tests * beginning of serializer test for hot phase * added serialization tests for form lib components * fix some i18n issues * fixed delete phase integration test * move hot phase serialization test to pre-existing test location * fix another jest test issue * fix ui metric tracking for setting input priority in hot phase * refactor use rollover switch to form lib component and update validation for number segments in force merge * readded missing validation 🤦🏼‍♂️ * fix type check issues and setting of rollover enabled 🙄 * migrate all form lib components to spreading all rest props in EuiFormRow * added comment to test helper function * refactor test helper setPhaseIndexPriorityFormLib -> setPhaseIndexPriority * refactor to use form schema * Removed use of UseMultiFields component - also fix missing "key" on react component in unrelated file - fixed ordering of JSON in test file - also removed default value from form schema so that when a value is not set for max size, max docs or max age it will remain unset in future policies * update json flyout behaviour * fix json policy serialization * Fix type and i18n issues * do not use form.subscribe * add missing key value in cells Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

jloleysens added 4 commits October 6, 2020 17:02

wip

0b59256

added missing shared_imports file to index

ea04a60

initial migration of hot phase to form lib

5f265af

- tests are now broken - need to break up the hot_phase file in to meaningful parts - duplicated set_priority and forcemerge components

Big refactor

16adb9b

- moved a lot of files around - removed the need for the state to track whether rollover is set

jloleysens added chore Feature:ILM v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 8, 2020

kibanamachine and others added 11 commits October 12, 2020 05:56

Merge branch 'master' into ilm/migrate-hotphase-to-formlib

4cd11d6

Integrate form lib serialization with existing serialization

0eb0610

- refactor serializePolicy -> legacySerializePolicy - updated serialization of form lib to factor in pre-existing policy values. These should be interacted with in a non-lossy way.

wip on fixing jest tests and some other refactors

d9f4c5f

fix jest tests and other refactors

f030d70

delete existing hot phase serialization and tests

deb67bf

beginning of serializer test for hot phase

d087b44

added serialization tests for form lib components

bed6317

fix some i18n issues

3655f0f

fixed delete phase integration test

5dc6023

move hot phase serialization test to pre-existing test location

d1fdd66

jloleysens requested a review from sebelga October 13, 2020 16:47

jloleysens added 2 commits October 13, 2020 19:18

fix another jest test issue

a33ab32

fix ui metric tracking for setting input priority in hot phase

2cbc917

jloleysens marked this pull request as ready for review October 14, 2020 07:26

jloleysens requested a review from a team as a code owner October 14, 2020 07:26

Merge branch 'master' into ilm/migrate-hotphase-to-formlib

f78b566

sebelga reviewed Oct 15, 2020

View reviewed changes

kibanamachine and others added 10 commits October 16, 2020 04:19

Merge branch 'master' into ilm/migrate-hotphase-to-formlib

eab1538

fix type check issues and setting of rollover enabled 🙄

a8ab7b6

migrate all form lib components to spreading all rest props in EuiFor…

b7a7f37

…mRow

added comment to test helper function

759a0a0

refactor test helper setPhaseIndexPriorityFormLib -> setPhaseIndexPri…

100ad62

…ority

refactor to use form schema

9d869c8

update json flyout behaviour

7da30bd

fix json policy serialization

7f48a65

Fix type and i18n issues

e5665d0

Merge branch 'master' into ilm/migrate-hotphase-to-formlib

55346ba

sebelga reviewed Oct 19, 2020

View reviewed changes

do not use form.subscribe

b560f21

add missing key value in cells

371ff40

sebelga reviewed Oct 19, 2020

View reviewed changes

sebelga approved these changes Oct 19, 2020

View reviewed changes

jloleysens merged commit 30fc4be into elastic:master Oct 19, 2020

jloleysens deleted the ilm/migrate-hotphase-to-formlib branch October 19, 2020 15:34

jloleysens mentioned this pull request Oct 19, 2020

[7.x] [ILM] Migrate Hot phase to Form Lib (#80012) #81013

Merged

This was referenced Oct 21, 2020

[ILM] Migrate Warm phase to Form Lib #81323

Merged

[ILM] Migrate Cold phase to Form Lib #81754

Merged

jloleysens mentioned this pull request Nov 6, 2020

[ILM] Migrate Delete phase and name field to Form Lib #82834

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILM] Migrate Hot phase to Form Lib #80012

[ILM] Migrate Hot phase to Form Lib #80012

jloleysens commented Oct 8, 2020 •

edited

Loading

jloleysens commented Oct 12, 2020

jloleysens commented Oct 14, 2020

elasticmachine commented Oct 14, 2020

sebelga left a comment

sebelga Oct 15, 2020

sebelga Oct 15, 2020

sebelga Oct 15, 2020

sebelga Oct 15, 2020

sebelga Oct 15, 2020

sebelga Oct 15, 2020

sebelga Oct 15, 2020

jloleysens Oct 16, 2020

sebelga Oct 16, 2020

sebelga Oct 15, 2020

jloleysens Oct 16, 2020

sebelga Oct 15, 2020

sebelga Oct 15, 2020

jloleysens commented Oct 16, 2020

jloleysens commented Oct 19, 2020

sebelga Oct 19, 2020

sebelga Oct 19, 2020 •

edited

Loading

jloleysens Oct 19, 2020

jloleysens commented Oct 19, 2020

kibanamachine commented Oct 19, 2020

sebelga Oct 19, 2020

sebelga Oct 19, 2020 •

edited

Loading

jloleysens Oct 19, 2020

sebelga left a comment

		describedByIds={rest.idAria ? [rest.idAria] : undefined}
		{...rest}


		<EuiSpacer />

		<HotPhase setWarmPhaseOnRollover={setWarmPhaseOnRollover} />

[ILM] Migrate Hot phase to Form Lib #80012

[ILM] Migrate Hot phase to Form Lib #80012

Conversation

jloleysens commented Oct 8, 2020 • edited Loading

Summary

How to review

New JSON policy flyout behaviour

Checklist

jloleysens commented Oct 12, 2020

jloleysens commented Oct 14, 2020

elasticmachine commented Oct 14, 2020

sebelga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Oct 16, 2020

jloleysens commented Oct 19, 2020

Choose a reason for hiding this comment

sebelga Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Oct 19, 2020

kibanamachine commented Oct 19, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

Choose a reason for hiding this comment

sebelga Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga left a comment

Choose a reason for hiding this comment

jloleysens commented Oct 8, 2020 •

edited

Loading

sebelga Oct 19, 2020 •

edited

Loading

sebelga Oct 19, 2020 •

edited

Loading