Skip to content
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

[ILM] Migrate Warm phase to Form Lib #81323

Merged

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Oct 21, 2020

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.

@jloleysens 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
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

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.
@jloleysens jloleysens changed the title [ILM] Migrate warm phase to formlib [ILM] Migrate Warm phase to Form Lib Oct 21, 2020
@jloleysens jloleysens marked this pull request as ready for review October 22, 2020 07:36
@jloleysens jloleysens requested review from a team as code owners October 22, 2020 07:36
@elasticmachine
Copy link
Contributor

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

@jloleysens
Copy link
Contributor Author

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

which has already received design review.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Comment on lines +1 to +2
.indexLifecycleManagement__phase__dataTierAllocation {
&__controlSection {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Screen Shot 2020-10-22 at 15 30 18 PM

Copy link
Contributor Author

@jloleysens jloleysens Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you could use the form helper from the testBed object

form.setInputValue(dataTestSubject, value);

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

const enable = (phase: string) => createFormClickAction(`enablePhaseSwitch-${phase}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

const setReplicas = async (value: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

watch: [allocationNodeAttributePath],
});

const selectedAllocationNodeAttribute = get(formData, allocationNodeAttributePath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getPolicy or updatePolicy? 😊

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

import { FormInternal, DataAllocationMetaFields } from './types';
import { isNumber } from '../../services/policies/policy_serialization';

const unsafeSerializePhaseWithAllocation = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will refactor to make it a pure function instead!

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SASS is ok. Could user better naming.

Comment on lines +1 to +2
.indexLifecycleManagement__phase__dataTierAllocation {
&__controlSection {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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 jloleysens merged commit 1712f0d into elastic:master Oct 27, 2020
@jloleysens jloleysens deleted the ilm/migrate-warm-phase-to-formlib branch October 27, 2020 08:43
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 27, 2020
* 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>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
* 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)
  ...
jloleysens added a commit that referenced this pull request Oct 27, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ILM 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants