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 Delete phase and name field to Form Lib #82834

Merged

Conversation

jloleysens
Copy link
Contributor

Summary

Migrated the delete phase to use the form lib. Continuation of: #80012 #81323 #81754

How to review

This PR removes a lot of duplicated code introduced int he PRs referenced above as well as all of the legacy deserialization and serialization logic.

The primary focus on migrating is for the delete phase and so most of the work is for the "wait_for_snapshot" field.

Checklist

Delete any items that are not applicable to this PR.

@jloleysens jloleysens requested review from a team as code owners November 6, 2020 14:21
@jloleysens jloleysens added 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 labels Nov 6, 2020
@elasticmachine
Copy link
Contributor

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

getUrlForApp,
}}
>
<PresentationComponent history={history} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move history prop to context for consistency or alternatively, pass isNewPolicy, existingPolicies and policyName as props too. I think they are only used in edit_policy.tsx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but would like to hold off on this change because it seems cleaner to me to have the EditPolicyContextProvider contain the values for the policy business logic. Most of those values are relevant to the "name" field of the policy and I would like to preserve the flexibility of being able to move that field into its own component if needed.

@yuliacech
Copy link
Contributor

Hi @jloleysens , great job on converting the rest of edit policy to form lib! I tested the delete phase locally and all worked for me, with and without snapshot policies.
The code looks good, especially deleting all of that duplicated lines!
I would like to block on 'Fix errors' badge, which is now shown on all phases regardless of where the actual validation error is and even when the phase is inactive. I think that could be confusing for the user.
Remaining non-blocking comments are about code organization:

  • Would you consider creating a form lib related folder in edit_policy? For me, the number of files in the root of edit_policy seems a bit on the larger side.
  • Would you consider renaming edit_policy.container.tsx? I think the name is left over from when Redux was used and still has its specific meaning behind it. I would suggest edit_policy.tsx for 'container' file and edit_policy_form for the actual file where useForm is called.

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

LGTM

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@yuliacech Thanks for the review!

which is now shown on all phases regardless of where the actual validation error is and even when the phase is inactive. I think that could be confusing for the user.

Great catch 🍻

I have done the following:

  • since the "fix errors" badge was to improve visibility of where errors are after attempting to save a policy, I though it is best to remove this since errors are now reported as the user is editing that field.
  • I moved form-related files into the a directory form in the edit_policy section
  • w.r.t. edit_policy.container.tsx I'd like to hold off on this change because I know there are other plugins where this convention has been followed (like ingest pipelines) and I think we should decide with the team what the best approach here would be. IMO name.container still does a good job of communicating what the intent of that file is - there is no presentational logic, just setting up of context and preparing props for the presentation component which is common in the flux architecture in React.

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, thank you for addressing my feedback! Changes LGTM, re-tested locally, all worked.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
indexLifecycleManagement 112 110 -2

async chunks size

id before after diff
indexLifecycleManagement 224.4KB 210.6KB -13.8KB

page load bundle size

id before after diff
indexLifecycleManagement 77.0KB 76.8KB -188.0B

History

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

@jloleysens jloleysens merged commit f2f76e1 into elastic:master Nov 9, 2020
@jloleysens jloleysens deleted the ilm/migrate-delete-phase-to-formlib branch November 9, 2020 13:29
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 9, 2020
* remove use of legacy state system and legacy serialization

* remove legacy min_age input component and re-add missing import

* rename shared -> shared_fields for more clarity

* some more cleanup and fixing regressions on policy name for creating new policy from existing policy

* move extract policy static code to lib folder and remove "policies" dir from services

* fix jest tests and minor policy flyout inconsistency

* remove legacy helper

* fix client integration tests

* fix min for set index priority

* moved save policy function into edit policy section

* remove unused translations

* refactor form files to own edit_policy/form folder

* remove "fix errors" badge to fix UX - users can see errors in line before pressing save so the value of this badge has diminished

* fix i18n after removing phase error badge

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 9, 2020
* master:
  Added `defaultActionMessage` to index threshold alert UI type definition (elastic#80936)
  [ILM] Migrate Delete phase and name field to Form Lib (elastic#82834)
  skip flaky suite (elastic#57426)
  [Alerting] adds an Run When field in the alert flyout to assign the action to an Action Group (elastic#82472)
  [APM] Expose APM event client as part of plugin contract (elastic#82724)
  [Logs UI] Fix errors during navigation (elastic#78319)
  Enable send to background in TSVB (elastic#82835)
  SavedObjects search_dsl: add match_phrase_prefix clauses when using prefix search (elastic#82693)
  [Ingest Manager] Unify install* under installPackage (elastic#82916)
jloleysens added a commit that referenced this pull request Nov 9, 2020
* remove use of legacy state system and legacy serialization

* remove legacy min_age input component and re-add missing import

* rename shared -> shared_fields for more clarity

* some more cleanup and fixing regressions on policy name for creating new policy from existing policy

* move extract policy static code to lib folder and remove "policies" dir from services

* fix jest tests and minor policy flyout inconsistency

* remove legacy helper

* fix client integration tests

* fix min for set index priority

* moved save policy function into edit policy section

* remove unused translations

* refactor form files to own edit_policy/form folder

* remove "fix errors" badge to fix UX - users can see errors in line before pressing save so the value of this badge has diminished

* fix i18n after removing phase error badge

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 10, 2020
…e-details-overlay

* 'master' of github.com:elastic/kibana: (201 commits)
  Added `defaultActionMessage` to index threshold alert UI type definition (elastic#80936)
  [ILM] Migrate Delete phase and name field to Form Lib (elastic#82834)
  skip flaky suite (elastic#57426)
  [Alerting] adds an Run When field in the alert flyout to assign the action to an Action Group (elastic#82472)
  [APM] Expose APM event client as part of plugin contract (elastic#82724)
  [Logs UI] Fix errors during navigation (elastic#78319)
  Enable send to background in TSVB (elastic#82835)
  SavedObjects search_dsl: add match_phrase_prefix clauses when using prefix search (elastic#82693)
  [Ingest Manager] Unify install* under installPackage (elastic#82916)
  [Fleet] Make stream id unique in agent policy (elastic#82447)
  skip flaky suite (elastic#82915)
  skip flaky suite (elastic#75794)
  Copy `dateAsStringRt` to observability plugin (elastic#82839)
  [Maps] rename connected_components/map folder to mb_map (elastic#82897)
  [Security Solution] Fix EventsViewer DnD cypress tests (elastic#82619)
  [Security Solution] Adds logging and performance fan out API for threat/Indicator matching (elastic#82546)
  Implemented Alerting health status pusher by using task manager and status pooler for Kibana status plugins 'kibanahost/api/status' (elastic#79056)
  [APM] Adds new configuration 'xpack.apm.maxServiceEnvironments' (elastic#82090)
  Move single use function in line (elastic#82885)
  [ML] Add unsigned_long support to data frame analytics and anomaly detection (elastic#82636)
  ...
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