[SR] Selected indices do not show when editing a policy created via console by jkelastic · Pull Request #55219 · elastic/kibana

jkelastic · 2020-01-17T19:28:32Z

Summary

Fixes #47156 Rename index pattern to custom pattern and default to customer pattern page

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Release note

This PR resolves an issue where the indices of a Snapshot Lifecycle Policy were not always displayed correctly in the Snapshot & Restore UI when editing an existing policy.

elasticmachine · 2020-01-17T20:34:08Z

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

alisonelizabeth

@jkelastic great start with this! I think there's still some more work to be done here around the logic determining when to show the custom vs. list view.

x-pack/legacy/plugins/snapshot_restore/public/index.ts

...ck/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_settings.tsx

jkelastic · 2020-02-11T20:53:08Z

@elasticmachine merge upstream

jkelastic · 2020-02-14T16:50:07Z

@alisonelizabeth I need some help fixing the issues. Not sure what to do.

…ot_restore_custom_pattern

alisonelizabeth · 2020-02-18T20:54:01Z

@sebelga would you mind reviewing/testing this one? I’ve contributed with @jkelastic, so I’d like another set of eyes on it.

While debugging this issue, I discovered a SLM policy may have indices defined as a comma-separated string (per the docs) or an array. In the UI, we were sending the data back differently depending if the user selected indices using the list or combobox. IMO this caused some unnecessary complexity, as there was logic to handle both cases to some extent, but even this wasn’t working 100%. I updated the deserialization function to convert the indices to an array if it is defined as a string. The UI will also only ever send back an array now. This should address the original bug outlined in #47156 and also clean up some of the logic.

…ot_restore_custom_pattern

sebelga

I am still seeing the bug when opening the settings tab. It says 2 indices will be backed up but actually none are selected.

I also saw another bug, maybe it deserves another PR maybe not. When I select 1 index, then I unselect the index, the tab is invalid and the "Next" button is disabled. But I can still click on the "Logistic" step. At that moment I am blocked and can't do anything and need to cancel the editing.

sebelga · 2020-02-24T09:25:58Z

...ck/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_settings.tsx

+    return Array.isArray(config.indices) && config.indices.includes(index);
+  });
+
+  const policyIndicesLength = Array.isArray(config.indices) && config.indices.length;


This will return false if config.indices is not an Array.
We probably want something like this

const policyIndicesLength = Array.isArray(config.indices) ? config.indices.length : 0;

Also, isn't config.indices always an Array? Why do we need this check?

@sebelga It was not always an Array and then we created the check to see if it was. Then we went ahead to converted the indices to an Array (always an Array) and forgot to remove the check. I can go ahead and remove it. Thanks.

To clarify - a user is not required to define indices, so it is possible this will be undefined.

sebelga · 2020-02-24T10:59:29Z

...ck/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_settings.tsx

+  // Policy indices (config.indices) can contain existing indices, index patterns, aliases or non-existing indices
+  // This returns the policy indices that exist in a user's system
+  const policyIndicesInAllIndices = indices.filter(index => {
+    return Array.isArray(config.indices) && config.indices.includes(index);


Isn't config.indices always an Array? Why do we need this check?

If we need the check, it would be better outside the loop

const policyIndicesInAllIndices = Array.isArray(config.indices) ? indices.filter(index => config.indices!.includes(index)) : [];

@sebelga Same as above, I will remove this.

sebelga · 2020-02-24T11:00:50Z

...ck/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_settings.tsx

-                selectIndicesMode === 'custom'
-                  ? indexPatterns.join(',')
-                  : [...(indicesSelection || [])],
+                selectIndicesMode === 'custom' ? [...indexPatterns] : [...(indicesSelection || [])],


It seems like indicesSelection is always an Array, do we need the extra || [] ?

sebelga · 2020-02-24T11:11:57Z

In the file collapsible_indices_list.tsx the Props should be

interface Props {
  indices?: string[];
}

jkelastic · 2020-02-25T23:25:49Z

I am still seeing the bug when opening the settings tab. It says 2 indices will be backed up but actually none are selected.

@sebelga You're correct. We didn't address this bug in the ticket. I didn't know if we're suppose to. We only address whether the page loads to the "customer index pattern" or "selected index" if the index is in the policy itself. Do we need to create a separate PR ticket for this?

I also saw another bug, maybe it deserves another PR maybe not. When I select 1 index, then I unselect the index, the tab is invalid and the "Next" button is disabled. But I can still click on the "Logistic" step. At that moment I am blocked and can't do anything and need to cancel the editing.

I will need to take a look at this. Great found.

sebelga · 2020-02-26T08:32:58Z

Do we need to create a separate PR ticket for this?

I think the bug should be addressed in this PR as the PR description says that this fixes #47156. Thanks 👍 !

sebelga · 2020-02-26T09:08:20Z

@jkelastic Actually it would be better to not increase the fixes/changes in this PR as I am preparing a PR that will move all the SR files to the "plugins" folder.

So maybe update this PR description to specify exactly what it fixes (not 47156) and steps to reproduce.

This way, if I merge first, you can easily re-apply the changes on a new branch out of master.
If you merge first I will have to do the same, but it seems that it will be more complicated as I willl have many more changes.
So the lesser the changes, the better 👍

…ot_restore_custom_pattern

alisonelizabeth · 2020-02-27T20:45:33Z

I also saw another bug, maybe it deserves another PR maybe not. When I select 1 index, then I unselect the index, the tab is invalid and the "Next" button is disabled. But I can still click on the "Logistic" step. At that moment I am blocked and can't do anything and need to cancel the editing.

@sebelga @jkelastic I confirmed this bug exists on master, and I think should be addressed separately. The same problem occurs if I have an invalid field on the retention step and try to go back to a previous step. I opened #58773.

alisonelizabeth · 2020-02-28T17:07:32Z

@sebelga would you mind taking another look at this PR? I believe I addressed the bug you found, as well as your other review comments.

I also wanted to clarify that this PR is meant to fix #47156, so if there are any other issues found, they should be addressed. Thanks!

/cc @jkelastic

…ot_restore_custom_pattern

kibanamachine · 2020-02-28T22:20:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 69d1452

Test Failures

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/spaces/server/lib/request_interceptors.onRequestInterceptor requests proxied to the legacy platform handles paths without a space identifier

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 2 times on tracked branches: https://github.com/elastic/kibana/issues/58941

Stack Trace

: Timeout - Async callback was not invoked within the 30000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 30000ms timeout specified by jest.setTimeout.Error: 
    at new Spec (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
    at new Spec (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/setup_jest_globals.js:80:9)
    at specFactory (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:575:24)
    at Env.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:644:24)
    at Env.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:132:23)
    at it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/jasmineLight.js:93:21)
    at Suite.describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.test.ts:129:5)
    at addSpecsToSuite (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:496:51)
    at Env.describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:466:11)
    at describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/jasmineLight.js:81:18)
    at Suite.Object.<anonymous>.describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.test.ts:128:3)
    at addSpecsToSuite (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:496:51)
    at Env.describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/Env.js:466:11)
    at describe (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmine/jasmineLight.js:81:18)
    at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/spaces/server/lib/request_interceptors/on_request_interceptor.test.ts:21:1)
    at Runtime._execModule (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runtime/build/index.js:867:68)
    at Runtime._loadModule (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runtime/build/index.js:577:12)
    at Runtime.requireModule (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runtime/build/index.js:433:10)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:202:13
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:27:24)
    at _next (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:47:9)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:52:7
    at new Promise (<anonymous>)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:44:12
    at jasmine2 (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/index.js:60:19)
    at /var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:385:24
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:161:24)
    at _next (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:181:9)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/spaces/server/lib/request_interceptors.onRequestInterceptor requests proxied to the legacy platform strips the Space URL Context from the request

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 2 times on tracked branches: https://github.com/elastic/kibana/issues/58942

Stack Trace

Error: Http server is not setup up yet
    at HttpServer.start (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/src/core/server/http/http_server.ts:142:13)
    at HttpService.start (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/src/core/server/http/http_service.ts:134:29)

History

💔 Build #29784 failed ffa8b48
💔 Build #27979 failed e1e1bd2
💚 Build #27530 succeeded f29abd3
💔 Build #27515 failed f4d3e1b
💔 Build #27468 failed 819efac

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

alisonelizabeth · 2020-03-05T14:02:25Z

@jkelastic now that #59109 is merged, can you work to update this branch and resolve the merge conflicts? Let me know if you need help.

sebelga · 2020-03-05T14:43:43Z

@jkelastic I think it would be easier to create a branch from master an apply the change from the 8 files modified in this PR.

jkelastic · 2020-03-05T21:58:50Z

@jkelastic I think it would be easier to create a branch from master an apply the change from the 8 files modified in this PR.

thanks, closing this for #59486

jkelastic added v7.7.0 v8.0.0 release_note:fix Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// labels Jan 17, 2020

alisonelizabeth requested changes Jan 21, 2020

View reviewed changes

x-pack/legacy/plugins/snapshot_restore/public/index.ts Outdated Show resolved Hide resolved

...ck/legacy/plugins/snapshot_restore/public/app/components/policy_form/steps/step_settings.tsx Outdated Show resolved Hide resolved

Set Combo value with policy value

8f18841

jkelastic force-pushed the bugfix/Snapshot_restore_custom_pattern branch from 9d273cd to 8f18841 Compare February 13, 2020 19:50

Fix precommit hook

5d06831

alisonelizabeth added 3 commits February 18, 2020 09:43

Merge branch 'master' of github.com:elastic/kibana into bugfix/Snapsh…

75e41c7

…ot_restore_custom_pattern

add deserialization for policy indices defined as strings

5cbb650

add comment

819efac

alisonelizabeth requested a review from sebelga February 18, 2020 20:54

alisonelizabeth added 3 commits February 18, 2020 17:55

address TS errors

f4d3e1b

i18n fix

f29abd3

Merge branch 'master' of github.com:elastic/kibana into bugfix/Snapsh…

e1e1bd2

…ot_restore_custom_pattern

sebelga suggested changes Feb 24, 2020

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into bugfix/Snapsh…

3aec49a

…ot_restore_custom_pattern

address review feedback

ffa8b48

Merge branch 'master' of github.com:elastic/kibana into bugfix/Snapsh…

e2d9738

…ot_restore_custom_pattern

fix TS error

69d1452

jkelastic closed this Mar 5, 2020

Conversation

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

Uh oh!

Summary

Checklist

For maintainers

Release note

Uh oh!

elasticmachine commented Jan 17, 2020

Uh oh!

alisonelizabeth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkelastic commented Feb 11, 2020

Uh oh!

jkelastic commented Feb 14, 2020

Uh oh!

alisonelizabeth commented Feb 18, 2020

Uh oh!

sebelga left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga commented Feb 24, 2020

Uh oh!

jkelastic commented Feb 25, 2020

Uh oh!

sebelga commented Feb 26, 2020

Uh oh!

sebelga commented Feb 26, 2020

Uh oh!

alisonelizabeth commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alisonelizabeth commented Feb 28, 2020

Uh oh!

kibanamachine commented Feb 28, 2020

💔 Build Failed

Standard Out

Stack Trace

Standard Out

Stack Trace

History

Uh oh!

alisonelizabeth commented Mar 5, 2020

Uh oh!

sebelga commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkelastic commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkelastic commented Jan 17, 2020 •

edited by alisonelizabeth

Loading

alisonelizabeth commented Feb 27, 2020 •

edited

Loading

sebelga commented Mar 5, 2020 •

edited

Loading