Kibana should allow a min_age setting of 0ms in ILM policy phases by jkelastic · Pull Request #53719 · elastic/kibana

jkelastic · 2019-12-20T19:00:34Z

Release note

The Index Lifecycle Policy editor now permits the minimum age to be 0.

elasticmachine · 2019-12-20T19:25:11Z

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

cjcenizal

I am so stoked to see this get fixed! Thanks @jkelastic.

Can we improve the UX by setting the default value for "min age" to 0? This requires changing three files: store/defaults/cold_phase.js (line 20), store/defaults/warm_phase.js (line 26), and store/defaults/cold_phase.js (line 18). All three lines need to be changed to:

[PHASE_ROLLOVER_MINIMUM_AGE]: 0,

Now when you go into the form, all of these inputs should be prepopulated with 0 and you should be able to save the form.

kibanamachine · 2019-12-21T00:48:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 231071a

History

💔 Build #16993 failed 809b32f

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

jkelastic · 2019-12-21T06:05:45Z

I am so stoked to see this get fixed! Thanks @jkelastic.

Can we improve the UX by setting the default value for "min age" to 0? This requires changing three files: store/defaults/cold_phase.js (line 20), store/defaults/warm_phase.js (line 26), and store/defaults/cold_phase.js (line 18). All three lines need to be changed to:
[PHASE_ROLLOVER_MINIMUM_AGE]: 0,
Now when you go into the form, all of these inputs should be prepopulated with 0 and you should be able to save the form.

@cjcenizal No problem, I'm having a lot of fun and glad to be able to contribute to the EUI team. Third file you mentioned , do you mean store/defaults/delete_phase.js (line 18)?

I changed a few check conditions in the edit_policy.js to allow equal to and above 0. I also removed the warm phase test to check empty values as we've defaulted the input to be 0. Therefore, we'll always have a value and it wouldn't be empty.

…default values of [PHASE_ROLLOVER_MINIMUM_AGE]: 0 in [cold,warm,delete_phase].js. 3) Set phase[numberedAttribute] <= 0 in lifecycle.js

kibanamachine · 2019-12-21T07:27:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 3d23c4e

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

kibanamachine · 2019-12-22T02:24:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6393ae2

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

kibanamachine · 2019-12-27T19:10:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 595ab8f

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

kibanamachine · 2019-12-27T21:20:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1827fab

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

kibanamachine · 2019-12-27T23:27:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2f7c197

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

jkelastic · 2019-12-27T23:43:39Z

@elasticmachine merge upstream

kibanamachine · 2019-12-28T01:01:21Z

💚 Build Succeeded

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

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

jkelastic · 2019-12-30T17:55:01Z

@elasticmachine merge upstream

jkelastic · 2020-01-02T18:07:33Z

@elasticmachine merge upstream

alisonelizabeth

@jkelastic nice job! Tested locally and verified fix. I did have a couple questions about the code though if you could take a look. Thanks!

alisonelizabeth · 2020-01-02T20:11:24Z

x-pack/legacy/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.js

    });
  });
  describe('warm phase', () => {
-    test('should show number required error when trying to save empty warm phase', () => {


I think this is still a valid test. Is there a reason why you deleted it?

I deleted it because we set a default value of 0 from the request below. Therefore, I don't think the test is valid anymore as will not have an empty value.

I am so stoked to see this get fixed! Thanks @jkelastic.

Can we improve the UX by setting the default value for "min age" to 0? This requires changing three files: store/defaults/cold_phase.js (line 20), store/defaults/warm_phase.js (line 26), and store/defaults/delete_phase.js (line 18). All three lines need to be changed to:

[PHASE_ROLLOVER_MINIMUM_AGE]: 0,

Now when you go into the form, all of these inputs should be prepopulated with 0 and you should be able to save the form.

I believe the user could still delete the default value, in which case, the validation would still occur.

thank you, updated my code

x-pack/legacy/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.js

alisonelizabeth · 2020-01-02T20:25:07Z

x-pack/legacy/plugins/index_lifecycle_management/public/store/selectors/lifecycle.js

@@ -123,9 +130,9 @@ export const validatePhase = (type, phase, errors) => {
      } else if (


I was looking through the code again, and I'm wondering if this block of code is necessary. I don't think it will ever reach this point, as we're already checking for negative numbers on L121.

} else if (phase[numberedAttribute] < 0) { phaseErrors[numberedAttribute] = [positiveNumberRequiredMessage]; }

Can you confirm?

It will reach this point. Initially I thought the negative numbers on L121 would have caught it too, but it didn't.

Can you share how you were testing it to reach this?

Sure, no problem. Initially I set <=0

} else if (phase[numberedAttribute] <= 0) { phaseErrors[numberedAttribute] = [positiveNumberRequiredMessage]; }

This will generate an error when I enter 0 through the Index Lifecycle Policy cold phase page. It was still not permitting 0. When I set < 0

} else if (phase[numberedAttribute] < 0) { phaseErrors[numberedAttribute] = [positiveNumberRequiredMessage]; }

I will not get an error when I enter 0 on the ILP page on the cold phase. Then when I run the jest test I will get a received error of 0 as 0 is now a valid value, but I will get anexpected error of 1.

expect(received).toBe(expected) // Object.is equality Expected: 1 Received: 0 74 | const expectedErrorMessages = (rendered, expectedErrorMessages) => { 75 | const errorMessages = rendered.find('.euiFormErrorText'); > 76 | expect(errorMessages.length).toBe(expectedErrorMessages.length); | ^ 77 | expectedErrorMessages.forEach(expectedErrorMessage => { 78 | let foundErrorMessage; 79 | for (let i = 0; i < errorMessages.length; i++) {

Therefore, I edited the jest test case to allow no error to be returned by setting as the value 0 is permitted now

expectedErrorMessages(rendered, [])

Sorry, I wasn't very clear in my comment. I was referring to your statement:

Initially I thought the negative numbers on L121 would have caught it too, but it didn't.

I was wondering what steps you were taking in the UI to reach this point. I am not able to reproduce.

Sorry, my misunderstanding. Fixed the issue now.

…nditions from lifecycle.js & the positiveNumbersEqualAboveZeroErrorMessage variable

jkelastic · 2020-01-09T19:33:31Z

@elasticmachine merge upstream

alisonelizabeth · 2020-01-10T13:21:37Z

@elasticmachine merge upstream

alisonelizabeth · 2020-01-10T14:44:16Z

@elasticmachine merge upstream

kibanamachine · 2020-01-10T16:04:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1f06380

History

💔 Build #19269 failed c382972
💔 Build #19088 failed a2a5275
💚 Build #17621 succeeded b76b763
💚 Build #17449 succeeded 5765daa

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

alisonelizabeth

Latest changes LGTM. Thanks @jkelastic!

alisonelizabeth · 2020-01-10T16:24:16Z

@cjcenizal do you think you could take another look at @jkelastic's latest changes?

cjcenizal

Great work @jkelastic! Thanks for doing this.

jkelastic · 2020-01-10T21:51:35Z

Great work @jkelastic! Thanks for doing this.

No problem! Thanks for giving me the opportunity! I had a great time, fun, and learned lots!

…es (#53719) (#54499)

* master: (69 commits) [Graph] Fix various a11y issues (elastic#54097) Add ApplicationService app status management (elastic#50223) logs in one time (elastic#54447) Deprecate using `elasticsearch.ssl.certificate` without `elasticsearch.ssl.key` and vice versa (elastic#54392) [Optimizer] Fix a stack overflow with watch_cache when it attempts to delete very large folders. (elastic#54457) Security - Role Mappings UI (elastic#53620) [SIEM] [Detection engine] Permission II (elastic#54292) Allow User to Cleanup Repository from UI (elastic#53047) [Detection engine] Some UX for rule creation (elastic#54471) share specific instances of some ui packages (elastic#54079) [ML] APM modules configs for RUM Javascript and NodeJS (elastic#53792) [APM] Delay rendering invalid license notification (elastic#53924) [Graph] Improve error message on graph requests (elastic#54230) [ILM] Kibana should allow a min_age setting of 0ms in ILM policy phases (elastic#53719) Unit Tests for common/lib (elastic#53736) [Graph] Only show explorable fields (elastic#54101) remove linting rule exception for markdown (elastic#54232) [Monitoring] Fetch shard data more efficiently (elastic#54028) [Maps] Add hiddenLayers option to embeddable map input (elastic#54355) Pass termOrder and hasTermsAgg properties to serializeThresholdWatch function (elastic#54391) ...

Kibana should allow a min_age setting of 0ms in ILM policy phases

809b32f

cjcenizal self-requested a review December 20, 2019 19:24

cjcenizal mentioned this pull request Dec 20, 2019

[ILM] Kibana should allow a min_age setting of 0ms in ILM policy phases #53617

Closed

7 tasks

cjcenizal added Feature:ILM release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.6.0 v8.0.0 labels Dec 20, 2019

cjcenizal suggested changes Dec 20, 2019

View reviewed changes

Fixed jest by adding >= 0 in lifecycle.js line 133

231071a

elastic deleted a comment from elasticmachine Dec 21, 2019

1) Updated the jest conditions to allow equal to and above 0. 2) Set …

3d23c4e

…default values of [PHASE_ROLLOVER_MINIMUM_AGE]: 0 in [cold,warm,delete_phase].js. 3) Set phase[numberedAttribute] <= 0 in lifecycle.js

Fix Internationalization check, line 81 was the same as line 74

6393ae2

Updated <=0 to < 0 as this will not throw error line 133 lifecycle.js

595ab8f

Remove comments

1827fab

Updated the edit_policy.js test cases to accept 0 timing phase

2f7c197

Merge branch 'master' into bugfix/ILM

c0e0ba9

Merge branch 'master' into bugfix/ILM

5765daa

Merge branch 'master' into bugfix/ILM

b76b763

alisonelizabeth requested changes Jan 2, 2020

View reviewed changes

Add back test cases to edit_policy.test.js and removed operational co…

3b52342

…nditions from lifecycle.js & the positiveNumbersEqualAboveZeroErrorMessage variable

Merge branch 'master' into bugfix/ILM

a2a5275

Merge branch 'master' into bugfix/ILM

c382972

Merge branch 'master' into bugfix/ILM

1f06380

alisonelizabeth approved these changes Jan 10, 2020

View reviewed changes

cjcenizal approved these changes Jan 10, 2020

View reviewed changes

alisonelizabeth merged commit 63d0bf8 into elastic:master Jan 10, 2020

alisonelizabeth mentioned this pull request Jan 10, 2020

[7.x] [ILM] Kibana should allow a min_age setting of 0ms in ILM policy phases (#53719) #54499

Merged

alisonelizabeth added a commit that referenced this pull request Jan 11, 2020

[ILM] Kibana should allow a min_age setting of 0ms in ILM policy phas…

ce6d6b5

…es (#53719) (#54499)

cjcenizal mentioned this pull request Jan 24, 2020

Kibana Edit index lifecycle policy GUI doesn't allow 0 for delete phase timing #55768

Closed

		@@ -123,9 +130,9 @@ export const validatePhase = (type, phase, errors) => {
		} else if (

Conversation

jkelastic commented Dec 20, 2019 • edited by cjcenizal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release note

Uh oh!

elasticmachine commented Dec 20, 2019

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Dec 21, 2019

💔 Build Failed

History

Uh oh!

jkelastic commented Dec 21, 2019

Uh oh!

kibanamachine commented Dec 21, 2019

💔 Build Failed

Uh oh!

kibanamachine commented Dec 22, 2019

💚 Build Succeeded

Uh oh!

kibanamachine commented Dec 27, 2019

💔 Build Failed

Uh oh!

kibanamachine commented Dec 27, 2019

💔 Build Failed

Uh oh!

kibanamachine commented Dec 27, 2019

💚 Build Succeeded

Uh oh!

jkelastic commented Dec 27, 2019

Uh oh!

kibanamachine commented Dec 28, 2019

💚 Build Succeeded

Uh oh!

jkelastic commented Dec 30, 2019

Uh oh!

jkelastic commented Jan 2, 2020

Uh oh!

alisonelizabeth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkelastic Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkelastic commented Jan 9, 2020

Uh oh!

alisonelizabeth commented Jan 10, 2020

Uh oh!

alisonelizabeth commented Jan 10, 2020

Uh oh!

kibanamachine commented Jan 10, 2020

💚 Build Succeeded

History

Uh oh!

alisonelizabeth left a comment

Choose a reason for hiding this comment

Uh oh!

jkelastic commented Dec 20, 2019 •

edited by cjcenizal

Loading

jkelastic Jan 2, 2020 •

edited

Loading