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 Cold phase to Form Lib #81754

Merged

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Oct 27, 2020

Summary

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

How to review

  • The majority of new code is concerned with tests and UseField
  • A lot of previously duplicated code has been cleaned up (per the points above), no need to do an in-depth review here
  • Read through the new cold phase tests to make sure they make sense (__jest__/client_integration)
  • Legacy tests have minimal changes just to make them work again (__jest__/components)

How to test

Same as #80012

Checklist

@jloleysens jloleysens added 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 27, 2020
@jloleysens jloleysens requested a review from yuliacech October 27, 2020 12:10
@jloleysens jloleysens requested review from a team as code owners October 27, 2020 12:10
@elasticmachine
Copy link
Contributor

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

@yuliacech
Copy link
Contributor

Hi @jloleysens , great work on converting edit policy to form lib! Tested locally and all seems to work, just a couple of questions to confirm/discuss:

  1. do you think it would be possible/useful to type strings for path and key properties in UseField:
<UseField
        key={`phases.${phase}.actions.set_priority.priority`}
        path={`phases.${phase}.actions.set_priority.priority`}
...
  1. Before, when a policy is created without a cold phase and the cold phase is enabled later, it would not have any index priority set. Now index priority is set to 50, is that intended? (I think there was 2 different 'default' values for phases before, when creating a new policy and when enabling a phase on an existing policy)
  2. Not sure if that is a bug, but if I set min age to -1, save button is still enabled and 'Fix errors' badge only shown after the button is clicked. It seems to me that the intention in the code is to show the badge and to disable the button when the form is invalid. For me, form.isValid in form context is undefined, the error on the field itself is shown though.

@jloleysens
Copy link
Contributor Author

jloleysens commented Oct 28, 2020

Hey @yuliacech ! Thanks for the review!

  1. I think it could be useful, but at this stage with the spec all defined in one place and CITs that cover the runtime behaviour I think the class of bugs that typing this paths gives is not massive. That being said, I came up with a solution for what something like that could look like:
interface MyInterface {
  a: {
    b: {
      c: {
        d: string;
      }
    }
  }
}

class PathBuilder<T> {
  constructor(protected parent?: PathBuilder<any>) {}

  property: keyof T;

  next(property: keyof T) {
    this.property = property;
    return new PathBuilder<T[typeof property]>(this);
  }

  private getPath() {
    const path: string[] = [];
    let parent: PathBuilder<any> = this.parent;
    while (parent !== undefined && parent.property) {
      path.push(String(parent.property))
      parent = parent.parent;
    }
    return path.reverse();
  }

  toString() {
    return this.getPath().join('.')
  }

}

const p = new PathBuilder<MyInterface>();


console.log(p.next('a').next('b').next('c').next('d').toString());
// a.b.c.d

Is this what you had in mind?

  1. That is a good point! I had not checked that, perhaps it is best to leave this unset for now :)

  2. This is the expected form behaviour. We will be changing this form overall quite soon I expect, I don't think we should be spending more effort now getting on parity with older behaviour. The isValid value of the form is only set explicitly after validate has been run which only happens when a form is submitted (or it is called in code somewhere explicitly). Field level validators run every time a field value changes.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@yuliacech I addressed the default setting for index priority. Would you mind taking another look?

Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Hi @jloleysens , thanks a lot for addressing my questions! Really interesting suggestion for PathBuilder, maybe we could add something like this to a shared lib. As discussed, index priority was not consistent before, having 2 different default values depending on the policy being saved or not. Agree, that this PR would fix the consistency of default values in phases.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
indexLifecycleManagement 242 110 -132

async chunks size

id before after diff
indexLifecycleManagement 312.1KB 232.9KB -79.3KB

distributable file count

id before after diff
default 48109 48108 -1

page load bundle size

id before after diff
indexLifecycleManagement 90.3KB 89.9KB -325.0B

History

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

@jloleysens
Copy link
Contributor Author

Not blocking on review from elastic/kibana-design@ because this PR removes an SCSS file that was duplicated from a pre-existing file which was previously reviewed #76126

@jloleysens jloleysens merged commit ac70e1e into elastic:master Oct 28, 2020
@jloleysens jloleysens deleted the ilm/migrate-cold-phase-to-formlib branch October 28, 2020 16:10
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 28, 2020
* use form lib fields and start updating deserializer

* delete legacy data tier allocation field

* finished deserialization

* delete legacy serialization, validation and deserialization

* fix type issue and remove propertyOf for now

* fix legacy tests and create another number validator

* added serialization test coverage

* fix elastic#81697

* clean up remaining legacy tests and slight update to existing test

* remove legacy unused components

* fix copy to be clearer for more scenarios

* remove remaining coldphase interface use and clean up unused i18n

* update default index priority for cold phase

* updated cold phase index priority to 0

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit that referenced this pull request Oct 29, 2020
* use form lib fields and start updating deserializer

* delete legacy data tier allocation field

* finished deserialization

* delete legacy serialization, validation and deserialization

* fix type issue and remove propertyOf for now

* fix legacy tests and create another number validator

* added serialization test coverage

* fix #81697

* clean up remaining legacy tests and slight update to existing test

* remove legacy unused components

* fix copy to be clearer for more scenarios

* remove remaining coldphase interface use and clean up unused i18n

* update default index priority for cold phase

* updated cold phase index priority to 0

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
4 participants