[ILM] Migrate Warm phase to Form Lib #81323

jloleysens · 2020-10-21T14:20:12Z

Summary

Migrated the warm phase to use the form lib. Continuation of #80012

duplicated the data tiers component so that I have the same logic but can just inject the UseField's as needed (which is where all the extra code is coming from). This will be removed once cold phase is migrated.
added more serialisation tests!

How to test

Same as #80012

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

…rink and replicas fields

Copied the entire field for now duplicating all of the components

- also removed policy serialization tests for warm phase

jloleysens · 2020-10-21T14:21:09Z

@elasticmachine merge upstream

kibanamachine · 2020-10-21T14:21:11Z

merge conflict between base and head

…arm-phase-to-formlib * 'master' of github.com:elastic/kibana: (55 commits) [UX] Fix map color variance and apply proper filter for extended stats (elastic#81106) [User Experience] Use EuiSelect for percentiles instead of SuperSelect (elastic#81082) [DOCS] Add link for monitoring ssl settings (elastic#81057) [test] Await loading indicator in monitoring test (elastic#81279) [ILM] Minor copy and link additions to cloud CTA for cold phase (elastic#80512) [Mappings editor] Add scaled_float and date_range comp integration tests (elastic#81287) [Discover] Deangularize context.app (elastic#80851) [O11y Overview] Add code to display/hide UX section when appropriate (elastic#80873) [Discover] Extend DiscoverNoResults component to show different message on error (elastic#79671) Fix tagcloud word overlapping (elastic#81161) [Security Solution] Fixes flaky test rules (elastic#81040) Changed the code to avoid tech debt with hacky solutions after receiving comments on EUI issue reported about this problem. (elastic#81183) [Security Solution][All] Replace old markdown renderer with the new one (elastic#80301) Add namespaced version of the API call (elastic#81278) [ML] Data Frame Analytics: Fix race condition and support for feature influence legacy format. (elastic#81123) [Fleet] Fix POLICY_CHANGE action creation for new policy (elastic#81236) [Security Solution][Endpoint][Admin] Malware user notification checkbox (elastic#78084) [SecuritySolution][Unit Tests] - fix flakey unit test (elastic#81239) skip flaky suite (elastic#81264) [Maps] fix top-level Map page is called 'Kibana' (elastic#81238) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared/forcemerge_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase.tsx

- also minor refactor to use useCallback in policy flyout now that getFormData changes when the form data changes.

elasticmachine · 2020-10-22T07:36:56Z

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

jloleysens · 2020-10-22T07:41:06Z

elastic/kibana-design@ (I think this is the right handle?) the need for review here is a false positive. No new SCSS was introduced with these changes because the file is a copy of

kibana/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/data_tier_allocation/data_tier_allocation.scss

Line 1 in 62f34f2

.indexLifecycleManagement__phase__dataTierAllocation {

which has already received design review.

jloleysens · 2020-10-22T07:50:14Z

@elasticmachine merge upstream

cchaos · 2020-10-22T19:34:48Z

...icy/components/phases/shared/data_tier_allocation_field/components/data_tier_allocation.scss

+.indexLifecycleManagement__phase__dataTierAllocation {
+  &__controlSection {


You mentioned this is a copy/paste of a different file. I see there are now two files named the same and with the exact same selector and styles. Is there a reason not to reuse the component instead of duplicating?

Hi @cchaos ! Thanks for the review, this file will be removed in a subsequent PR which will be started directly after this PR. This work is part of enabling a bigger redesign for ILM and it is easier to manage and review the new code in stages.

[EDIT] By "this file" I am referring to the first file in your screenshot. The whole "data_tier_allocation" dir there will be removed.

👍 Gotcha. Then the only other suggestion I have is to use BEM naming with a 3-letter feature prefix like we do in other solutions. That would turn this selector into ilmDataTierAllocation__controlSection for simplicity.

jloleysens · 2020-10-26T08:31:36Z

@elasticmachine merge upstream

sebelga

Great job @jloleysens ! Tested locally and I haven't spot any regression. I left a few comments in the code but no blockers.

Regarding the tests:

I do find it harder to understand what exactly we expect to find in each on of the variations when we test against a snapshot.

So if, for example, we have a test that says "with forceMerged on" and the expect is something like expect(...).toContain({ // exactly what we test }) then it would be easier for me to see the nuances in the payload.

sebelga · 2020-10-26T09:09:32Z

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

+  function createFormSetValueAction<V = string>(dataTestSubject: string) {
+    return async (value: V) => {
+      await act(async () => {
+        find(dataTestSubject).simulate('change', { target: { value } });


Here you could use the form helper from the testBed object

form.setInputValue(dataTestSubject, value);

sebelga · 2020-10-26T09:20:17Z

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

+  const setIndexPriority = (phase: string) =>
+    createFormSetValueAction(`${phase}-phaseIndexPriority`);
+
+  const enable = (phase: string) => createFormClickAction(`enablePhaseSwitch-${phase}`);


nit: should we type the phase here and below? 'hot' | 'warm' | 'cold' | 'delete'

sebelga · 2020-10-26T09:26:27Z

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

+  const setSelectedNodeAttribute = (phase: string) =>
+    createFormSetValueAction(`${phase}-selectedNodeAttrs`);
+
+  const setReplicas = async (value: string) => {


What would happen if we call twice this method in a test? It seems that the first time the switch would be "on" and the second time "off" and we would not be able to set thevalue. I think this is why we had the option to pass the checked value originally (to "toggleForceMerge" for e.g.).

I think that for toggles we could use the handler from testBed

form.toggleEuiSwitch('warm-setReplicasSwitch', true);

WDYT?

sebelga · 2020-10-26T09:29:55Z

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

@@ -46,9 +47,9 @@ describe('<EditPolicy />', () => {
        await actions.hot.setMaxSize('123', 'mb');
        await actions.hot.setMaxDocs('123');
        await actions.hot.setMaxAge('123', 'h');
-        await actions.hot.toggleForceMerge(true);
+        await actions.hot.toggleForceMerge();


I think I do like to be explicit in the state we expect to get. So by passing true I know that I am activating the toggle and I can continue to read the test with that assumptions. By not passing the value, we have look up and count how many time the toggle has been called to calculate its current state (so the order of the statements impacts the test)

sebelga · 2020-10-26T09:49:22Z

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

+        await actions.warm.warmPhaseOnRollover();
+        await actions.savePolicy();
+        const latestRequest = server.requests[server.requests.length - 1];
+        expect(JSON.parse(JSON.parse(latestRequest.requestBody).body)).toMatchInlineSnapshot(`


Follow up of what we discussed in the sync about using inline snapshot.

Another drawback is that we don't have an object to use in several places.
Here for example we could export the object being validated and, in the api integration test, send it to our API endpoint. Everything should be green. If ES makes a change in the contract we would have to update the object to make the API integration test turn green. And then make some changes to our UI public code to make this test pass.

sebelga · 2020-10-26T10:45:42Z

...it_policy/components/phases/shared/data_tier_allocation_field/components/node_allocation.tsx

+    watch: [allocationNodeAttributePath],
+  });
+
+  const selectedAllocationNodeAttribute = get(formData, allocationNodeAttributePath);


I have a hard time understanding what this. Not sure if it's because of the var name or because it is something I am not seeing in the UI. And as searching for "allocationNodeAttributePath" does not yield any result I am king of stuck :)

I meant "kind" of stuck. Although I did feel like a 👑 ! 😄

sebelga · 2020-10-26T11:25:49Z

...it_policy/components/phases/shared/data_tier_allocation_field/data_tier_allocation_field.tsx

+        const renderNotice = () => {
+          switch (allocationType) {
+            case 'node_roles':
+              const isCloudEnabled = cloud?.isCloudEnabled ?? false;


nit: IMO this type of check should be done only once at start and persisted for all the consumers.

sebelga · 2020-10-26T11:31:18Z

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

-    })();
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [form, legacyPolicy, formData]);
+    getPolicy();


getPolicy or updatePolicy? 😊

Also it would be better to deconstruct the validate handler from form to not pass the whole object.

sebelga · 2020-10-26T11:43:21Z

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

+import { FormInternal, DataAllocationMetaFields } from './types';
+import { isNumber } from '../../services/policies/policy_serialization';
+
+const unsafeSerializePhaseWithAllocation = (


Do you call it unsafe because we mutate the objects? Could this method be converted to a pure function and return new objects? It seems that it would make the code easier to understand.

Will refactor to make it a pure function instead!

sebelga · 2020-10-26T11:57:05Z

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

@@ -4,40 +4,133 @@
 * you may not use this file except in compliance with the Elastic License.
 */



Question: is this file going to replace the "policy_serialization.ts" in services?

Yes, the plan is for policy_serialization to be replaced entirely

…arm-phase-to-formlib * 'master' of github.com:elastic/kibana: [Trigger Actions UI] Properly unmount app (elastic#81436) skip flaky suite (elastic#81576) skip flaky suite (elastic#78373) [Security Solution] Fix styling of EditDataProvider content (elastic#81456) [Search] Error Alignment 2 (elastic#80965) [APM] Unskip test (elastic#81574) [ML] Fix partition value selection on the Single Metric Viewer (elastic#81585) cleaning up expression service types (elastic#80643) Fix suggestions dropdown for query input (elastic#80990) [Usage collection] Make `schema` mandatory (elastic#79999) [ILM] Update show/hide data tier logic on cloud (elastic#81455) added brace import to advanced settings (elastic#81458) chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies

- refactor toggle click to take a boolean arg - refactor selection options in data tier component to use a func to get select options.

Also rather deconstruct the validate fn from the form object

…ction

- with jest tests see origin here: elastic#81642

kibanamachine · 2020-10-26T18:34:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b85f492

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`indexLifecycleManagement`	220	242	+22

async chunks size

id	before	after	diff
`indexLifecycleManagement`	269.9KB	312.1KB	+42.2KB

distributable file count

id	before	after	diff
`default`	48080	48081	+1

page load bundle size

id	before	after	diff
`indexLifecycleManagement`	90.6KB	90.3KB	-333.0B

History

💔 Build #83685 failed 750992e
💔 Build #83658 failed 7b2f0ba
💚 Build #83571 succeeded db3894d
💚 Build #83282 succeeded 271351e
💔 Build #83118 failed 2ed016f

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

cchaos

SASS is ok. Could user better naming.

cchaos · 2020-10-26T20:06:58Z

...icy/components/phases/shared/data_tier_allocation_field/components/data_tier_allocation.scss

+.indexLifecycleManagement__phase__dataTierAllocation {
+  &__controlSection {


👍 Gotcha. Then the only other suggestion I have is to use BEM naming with a 3-letter feature prefix like we do in other solutions. That would turn this selector into ilmDataTierAllocation__controlSection for simplicity.

* migrate all fields on warm phase except, data alloc, replicas and shrink * introduce edit policy context to share original policy and migrate shrink and replicas fields * Refactored biggest field; data allocation Copied the entire field for now duplicating all of the components * remove unused import * complete migration of new serialization * Remove last vestiges of legacy warm phase - also removed policy serialization tests for warm phase * fix existing test coverage and remove use of "none" for node attribute * added policy serialization tests * remove unused translations * Fix use of useFormData after update - also minor refactor to use useCallback in policy flyout now that getFormData changes when the form data changes. * fix import path * simplify serialization snapshot tests * type phases: string -> phases: Phases * Addressed some PR review items - refactor toggle click to take a boolean arg - refactor selection options in data tier component to use a func to get select options. * updated data tier callout logic after new changes * getPolicy -> updatePolicy Also rather deconstruct the validate fn from the form object * fix detection of migrate false and refactor serialization to pure function * fix type issue * fix for correctly detecting policy data tier type - with jest tests see origin here: elastic#81642 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (37 commits) [ILM] Migrate Warm phase to Form Lib (elastic#81323) [Security Solutions][Detection Engine] Fixes critical bug with error reporting that was doing a throw (elastic#81549) [Detection Rules] Add 7.10 rules (elastic#81676) [kbn/optimizer] ignore missing metrics when updating limits with --focus (elastic#81696) [SECURITY SOLUTIONS] Bugs overview page + investigate eql in timeline (elastic#81550) [Maps] fix unable to edit cluster vector styles styled by count when switching to super fine grid resolution (elastic#81525) Fixed migration issue for case specific actions, by extending email action migrator checks (elastic#81673) [CI] Preparation for APM tracking on CI (elastic#80399) [Home] Fixes Kibana app description order on home page and updates Canvas copy (elastic#80057) Make sure `to` is 'now' and not the same as `from` (elastic#81524) Nitpicking the 8.0 Breaking Change issue template (elastic#81678) [SECURITY_SOLUTION] Fix text on onboarding screen (elastic#81672) [data.search] Skip async search tests in build candidates and production builds (elastic#81547) Fix previousStartedAt by not changing when execution fails (elastic#81388) [Monitoring] Fix a couple of issues with the cpu usage alert (elastic#80737) Telemetry collection xpack to ts project references (elastic#81269) Elasticsearch: don't use url authentication for new client (elastic#81564) [App Search] Credentials: implement working flyout form (elastic#81541) Properly encode links to edit user page (elastic#81562) [Alerting UI] Don't wait for health check before showing Create Alert flyout (elastic#80996) ...

* migrate all fields on warm phase except, data alloc, replicas and shrink * introduce edit policy context to share original policy and migrate shrink and replicas fields * Refactored biggest field; data allocation Copied the entire field for now duplicating all of the components * remove unused import * complete migration of new serialization * Remove last vestiges of legacy warm phase - also removed policy serialization tests for warm phase * fix existing test coverage and remove use of "none" for node attribute * added policy serialization tests * remove unused translations * Fix use of useFormData after update - also minor refactor to use useCallback in policy flyout now that getFormData changes when the form data changes. * fix import path * simplify serialization snapshot tests * type phases: string -> phases: Phases * Addressed some PR review items - refactor toggle click to take a boolean arg - refactor selection options in data tier component to use a func to get select options. * updated data tier callout logic after new changes * getPolicy -> updatePolicy Also rather deconstruct the validate fn from the form object * fix detection of migrate false and refactor serialization to pure function * fix type issue * fix for correctly detecting policy data tier type - with jest tests see origin here: #81642 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

jloleysens added 8 commits October 20, 2020 15:01

migrate all fields on warm phase except, data alloc, replicas and shrink

d844b12

introduce edit policy context to share original policy and migrate sh…

e867ffe

…rink and replicas fields

Refactored biggest field; data allocation

d349eee

Copied the entire field for now duplicating all of the components

remove unused import

779534e

complete migration of new serialization

5bb0be9

Remove last vestiges of legacy warm phase

98bf14b

- also removed policy serialization tests for warm phase

fix existing test coverage and remove use of "none" for node attribute

0121411

added policy serialization tests

d7896d7

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

jloleysens added 3 commits October 21, 2020 17:01

remove unused translations

4f2a615

Fix use of useFormData after update

2ed016f

- also minor refactor to use useCallback in policy flyout now that getFormData changes when the form data changes.

jloleysens changed the title ~~[ILM] Migrate warm phase to formlib~~ [ILM] Migrate Warm phase to Form Lib Oct 21, 2020

jloleysens added the Feature:ILM label Oct 22, 2020

jloleysens requested a review from sebelga October 22, 2020 07:36

jloleysens marked this pull request as ready for review October 22, 2020 07:36

jloleysens requested review from a team as code owners October 22, 2020 07:36

fix import path

5aed5cf

Merge branch 'master' into ilm/migrate-warm-phase-to-formlib

271351e

cchaos reviewed Oct 22, 2020

View reviewed changes

Merge branch 'master' into ilm/migrate-warm-phase-to-formlib

db3894d

sebelga approved these changes Oct 26, 2020

View reviewed changes

jloleysens added 9 commits October 26, 2020 15:13

simplify serialization snapshot tests

1affb8e

type phases: string -> phases: Phases

97da20e

Addressed some PR review items

2befd67

- refactor toggle click to take a boolean arg - refactor selection options in data tier component to use a func to get select options.

updated data tier callout logic after new changes

96e45ae

getPolicy -> updatePolicy

cb03877

Also rather deconstruct the validate fn from the form object

fix detection of migrate false and refactor serialization to pure fun…

6109d3a

…ction

fix type issue

750992e

fix for correctly detecting policy data tier type

b85f492

- with jest tests see origin here: elastic#81642

cchaos approved these changes Oct 26, 2020

View reviewed changes

cjcenizal mentioned this pull request Oct 26, 2020

ILM allocation can't be set to "Off" #81682

Closed

jloleysens merged commit 1712f0d into elastic:master Oct 27, 2020

jloleysens deleted the ilm/migrate-warm-phase-to-formlib branch October 27, 2020 08:43

jloleysens mentioned this pull request Oct 27, 2020

[7.x] [ILM] Migrate Warm phase to Form Lib (#81323) #81735

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 Warm phase to Form Lib #81323

[ILM] Migrate Warm phase to Form Lib #81323

jloleysens commented Oct 21, 2020 •

edited

Loading

jloleysens commented Oct 21, 2020

kibanamachine commented Oct 21, 2020

elasticmachine commented Oct 22, 2020

jloleysens commented Oct 22, 2020

jloleysens commented Oct 22, 2020

cchaos Oct 22, 2020

jloleysens Oct 26, 2020 •

edited

Loading

cchaos Oct 26, 2020

jloleysens commented Oct 26, 2020

sebelga left a comment

sebelga Oct 26, 2020

sebelga Oct 26, 2020

sebelga Oct 26, 2020

sebelga Oct 26, 2020

sebelga Oct 26, 2020

sebelga Oct 26, 2020

sebelga Oct 26, 2020

sebelga Oct 26, 2020

sebelga Oct 26, 2020

sebelga Oct 26, 2020

jloleysens Oct 26, 2020

sebelga Oct 26, 2020

jloleysens Oct 26, 2020

kibanamachine commented Oct 26, 2020

cchaos left a comment

cchaos Oct 26, 2020

		.indexLifecycleManagement__phase__dataTierAllocation {
		&__controlSection {

		@@ -4,40 +4,133 @@
		* you may not use this file except in compliance with the Elastic License.
		*/

[ILM] Migrate Warm phase to Form Lib #81323

[ILM] Migrate Warm phase to Form Lib #81323

Conversation

jloleysens commented Oct 21, 2020 • edited Loading

Summary

How to test

Checklist

jloleysens commented Oct 21, 2020

kibanamachine commented Oct 21, 2020

elasticmachine commented Oct 22, 2020

jloleysens commented Oct 22, 2020

jloleysens commented Oct 22, 2020

Choose a reason for hiding this comment

jloleysens Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Oct 26, 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

kibanamachine commented Oct 26, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

distributable file count

page load bundle size

History

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Oct 21, 2020 •

edited

Loading

jloleysens Oct 26, 2020 •

edited

Loading