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] Revisit searchable snapshot field after new redesign #90793

Merged

Conversation

jloleysens
Copy link
Contributor

Summary

After merging #88671 we restructured the ILM policy form aggressively so that timing inputs would be top-level and all other fields hidden. This un-did the effort to keep searchable snapshot visible at a top level. This PR resurfaces searchable snapshots (SS) at a top level in the cold phase.

The pattern followed is the same as the "Wait for snapshot policy" on the delete phase.

Additionally, includes a fix for form error state not clearing when a field is unmounted. Also added a test for this.

searchable-snapshot-field-top-level

Checklist

Delete any items that are not applicable to this PR.

- the error state of the form would not clear correctly if the
  erroring field was unmounted. The logic for clearing form errors
  was also incorrectly using "keys" instead of "values".
- updated the width of wait for snapshot policy field to be the
  same as other fields
@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.12.0 labels Feb 9, 2021
@jloleysens jloleysens requested review from a team as code owners February 9, 2021 15:32
@elasticmachine
Copy link
Contributor

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

@jloleysens
Copy link
Contributor Author

@yuliacech I updated your styling in 3391770 to make the "Advanced settings" section sit a bit more flush to with the left of the comment panel. Let me know what you think! I assume it was added to offset the height diff of the button group?

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.

Thanks for adding this, @jloleysens ! Looks good to me, just left a comment about snapshot policy in delete phase.

Comment on lines +12 to +13
padding-top: $euiSizeS;
padding-bottom: $euiSizeS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this, @jloleysens !

options: policies,
singleSelection: { asPlainText: true },
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 singleSelection prop is still needed as policy name otherwise looks a bit different (not like a text and suggesting that multiple values are possible)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, this was definitely an accident 😅

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@yuliacech yuliacech self-requested a review February 10, 2021 09:07
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.

Thanks for adding singleSelection back to the wait for snapshot field, @jloleysens !
Other changes LGTM 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
indexLifecycleManagement 239.3KB 240.0KB +718.0B

History

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

Copy link
Contributor

@mdefazio mdefazio 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 jloleysens merged commit e17878e into elastic:master Feb 10, 2021
@jloleysens jloleysens deleted the ilm/revisit-searchable-snapshot-field branch February 10, 2021 13:25
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 10, 2021
…0793)

* moved searchable snapshot field out of cold phase accordian

* refactor styling to padding top and bottom to get advanced settings drop down to sit flush with side of panel

* Error clearing fix and cosmetic changes

- the error state of the form would not clear correctly if the
  erroring field was unmounted. The logic for clearing form errors
  was also incorrectly using "keys" instead of "values".
- updated the width of wait for snapshot policy field to be the
  same as other fields

* fix hook dependency causing clearError to be called

* slight improvement to component integration test

* re-add singleSelection to snapshot policiy field config

* refactored Phase component API and fixed typo in comment

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 10, 2021
* master: (99 commits)
  [Fleet] Use Fleet Server indices in the search bar (elastic#90835)
  [Search Sessions] added an info flyout to session management (elastic#90559)
  [ILM] Revisit searchable snapshot field after new redesign (elastic#90793)
  [Alerting] License Errors on Alert List View (elastic#89920)
  RFC Improve saved object migrations algorithm (elastic#84333)
  [Lens] (Accessibility) Fix focus on drag and drop actions (elastic#90561)
  Use new shortcut links to Fleet discuss forums. (elastic#90786)
  Do not generate an ephemeral encryption key in production. (elastic#81511)
  [Fleet] Use staging registry for snapshot builds (elastic#90327)
  Actually deleting x-pack/tsconfig.refs.json (elastic#90898)
  Add deprecation warning to all Beats CM pages. (elastic#90741)
  skip flaky suite (elastic#90136)
  Revert "Revert "[Metrics UI] Add Metrics Anomaly Alert Type (elastic#89244)"" (elastic#90889)
  remove ref to removed tsconfig file
  [core.logging] Uses host timezone as default (elastic#90368)
  [Maps] remove maps_file_upload plugin and fold public folder into file_upload plugin (elastic#90292)
  Revert "[Metrics UI] Add Metrics Anomaly Alert Type (elastic#89244)"
  [dev-utils/ci-stats] support disabling ship errors (elastic#90851)
  Prefix with / (elastic#90836)
  [Metrics UI] Add Metrics Anomaly Alert Type (elastic#89244)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 12, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

jloleysens added a commit that referenced this pull request Feb 15, 2021
…90937)

* moved searchable snapshot field out of cold phase accordian

* refactor styling to padding top and bottom to get advanced settings drop down to sit flush with side of panel

* Error clearing fix and cosmetic changes

- the error state of the form would not clear correctly if the
  erroring field was unmounted. The logic for clearing form errors
  was also incorrectly using "keys" instead of "values".
- updated the width of wait for snapshot policy field to be the
  same as other fields

* fix hook dependency causing clearError to be called

* slight improvement to component integration test

* re-add singleSelection to snapshot policiy field config

* refactored Phase component API and fixed typo in comment

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

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 15, 2021
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.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants