[SR] Delete and execute SLM policies by jen-huang · Pull Request #41934 · elastic/kibana

jen-huang · 2019-07-24T22:36:42Z

Summary

This PR adds the ability to delete and execute SLM policies from the UI. Executing a policy is referred to as "Run" (i.e. "Run policy") in the UI and this action means that a snapshot will be taken immediately using the configuration specified in the policy. Delete is self explanatory and uses our single/multi-delete pattern used in other apps.

Screenshots

Actions in table and details

.

Run and delete actions in table row

Bulk delete from selecting multiple rows

Run and delete actions in policy details

Delete policy(ies)

.

Single delete modal

Bulk delete modal

Run policy

Run policy modal

elasticmachine · 2019-07-24T22:36:44Z

Pinging @elastic/es-ui

jen-huang · 2019-07-24T22:39:37Z

x-pack/legacy/plugins/snapshot_restore/public/app/components/repository_delete_provider.tsx


  const deleteRepository = () => {
    const repositoriesToDelete = [...repositoryNames];
-    deleteRepositories(repositoriesToDelete).then(({ data: { itemsDeleted, errors }, error }) => {


while working on policy deletion, I found that for server errors such as 404 or 500, which don't return a data object, early destructuring here causes an undefined object error, so no toast notification is shown. I adjusted this for policy deletion and fixed it for other delete providers in this app such as here.

jen-huang · 2019-07-24T22:40:51Z

...gins/snapshot_restore/public/app/sections/home/policy_list/policy_details/policy_details.tsx

      aria-labelledby="srPolicyDetailsFlyoutTitle"
      size="m"
-      maxWidth={400}
+      maxWidth={550}


I bumped up the max width of all flyouts in this app as recent changes in EUI made them too narrow

elasticmachine · 2019-07-24T23:33:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alisonelizabeth

@jen-huang tested locally and everything LGTM. Left a couple minor comments.

alisonelizabeth · 2019-07-25T16:37:44Z

...gins/snapshot_restore/public/app/sections/home/policy_list/policy_details/policy_details.tsx

+                    return (
+                      <EuiButtonEmpty
+                        color="danger"
+                        data-test-subj="srPoicyDetailsDeleteActionButton"


typo here i think: srPoicyDetailsDeleteActionButton --> srPolicyDetailsDeleteActionButton

alisonelizabeth · 2019-07-25T16:38:19Z

...gins/snapshot_restore/public/app/sections/home/policy_list/policy_details/policy_details.tsx

+                        }
+                        fill
+                        color="primary"
+                        data-test-subj="srPoicyDetailsExecuteActionButton"


srPoicyDetailsExecuteActionButton --> srPolicyDetailsExecuteActionButton

alisonelizabeth · 2019-07-25T16:39:40Z

.../plugins/snapshot_restore/public/app/sections/home/policy_list/policy_table/policy_table.tsx

+                        aria-label={i18n.translate(
+                          'xpack.snapshotRestore.policyList.table.actionExecuteAriaLabel',
+                          {
+                            defaultMessage: 'Run policy `{name}`',


should this be

`Run policy '{name}'`

alisonelizabeth · 2019-07-25T16:45:46Z

.../plugins/snapshot_restore/public/app/sections/home/policy_list/policy_table/policy_table.tsx

+                        aria-label={i18n.translate(
+                          'xpack.snapshotRestore.policyList.table.actionDeleteAriaLabel',
+                          {
+                            defaultMessage: 'Delete policy `{name}`',


should this be

`Delete policy '{name}'`

elasticmachine · 2019-07-31T22:24:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-01T00:30:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Add single and bulk policy delete * Add policy execution * Remove early destructuring of provider request responses * Address PR feedback * Adjust policy requests for useRequest changes * Fix policy reload

cjcenizal

Looks awesome! I know this is a bit late, but I had some suggestions for your perusal.

cjcenizal · 2019-08-01T21:24:22Z

x-pack/legacy/plugins/snapshot_restore/public/app/components/policy_delete_provider.tsx

+  };
+
+  const deletePolicy = () => {
+    const policiesToDelete = [...policyNames];


Why do we create a copy of policyNames here? Does something bad happen if we delete policyNames directly?

cjcenizal · 2019-08-01T21:25:36Z

x-pack/legacy/plugins/snapshot_restore/public/app/components/policy_delete_provider.tsx

+  const deletePolicy = () => {
+    const policiesToDelete = [...policyNames];
+    deletePolicies(policiesToDelete).then(({ data, error }) => {
+      const { itemsDeleted, errors } = data || { itemsDeleted: undefined, errors: undefined };


Nit: we could also assign a default value to the data parameter above, which would be a bit terser. We could also set a default value to itemsDeleted and errors, which would make lines 52 and 72 terser as well.

deletePolicies(policiesToDelete).then(({ data = {}, error }) => { const { itemsDeleted = [], errors = [] } = data;

// ln 52 if (itemsDeleted.length) {

// ln 72 if (error || errors.length) {

I just realized that this won't quite work with the current logic for displaying error messages, but I have a suggestion that could make it work in the next comment.

cjcenizal · 2019-08-01T21:38:12Z

x-pack/legacy/plugins/snapshot_restore/public/app/components/policy_delete_provider.tsx

+      // Surface error notifications
+      // `error` is generic server error
+      // `data.errors` are specific errors with removing particular policy(ies)
+      if (error || (errors && errors.length)) {


I was having trouble following this logic because it contains two parallel conditional branches. How about something like this to make the two branches easier to discern and compare?

function createErrorMessageMultiple(count) { return i18n.translate('xpack.snapshotRestore.deletePolicy.errorMultipleNotificationTitle', { defaultMessage: 'Error deleting {count} policies', values: { count }, }); } function createErrorMessageSingle(name) { return i18n.translate('xpack.snapshotRestore.deletePolicy.errorSingleNotificationTitle', { defaultMessage: "Error deleting policy '{name}'", values: { name }, }); } // Surface error notifications let errorMessage; if (errors.length) { // `data.errors` are specific errors with removing particular policy(ies) errorMessage = errors.length > 1 ? createErrorMessageMultiple(errors.length) : createErrorMessageSingle(errors[0].name); } else if (error) { // `error` is generic server error errorMessage = policiesToDelete.length > 1 ? createErrorMessageMultiple(policiesToDelete.length) : createErrorMessageSingle(policiesToDelete[0]) } if (errorMessage) { toastNotifications.addDanger(errorMessage); }

cjcenizal · 2019-08-01T21:41:54Z

x-pack/legacy/plugins/snapshot_restore/public/app/components/repository_delete_provider.tsx

  const deleteRepository = () => {
    const repositoriesToDelete = [...repositoryNames];
-    deleteRepositories(repositoriesToDelete).then(({ data: { itemsDeleted, errors }, error }) => {
+    deleteRepositories(repositoriesToDelete).then(({ data, error }) => {


Can this be fixed like this?

// Default `data` to an empty object if it's not provided, which will have // undefined `itemsDeleted` and `errors` properties. deleteRepositories(repositoriesToDelete).then(({ data: { itemsDeleted, errors } = {}, error }) => {

cjcenizal · 2019-08-01T21:44:22Z

...gins/snapshot_restore/public/app/sections/home/policy_list/policy_details/policy_details.tsx

+                        onClick={() =>
+                          executePolicyPrompt(policyName, () => {
+                            onPolicyExecuted();
+                            reload();


Is this needed? It looks like policy_list calls reload itself.

cjcenizal · 2019-08-01T21:52:34Z

x-pack/legacy/plugins/snapshot_restore/server/routes/api/policy.ts

+
+export const deleteHandler: RouterRouteHandler = async (req, callWithRequest) => {
+  const { names } = req.params;
+  const policyNames = names.split(',');


Dumb question: are commas disallowed in policy names?

jen-huang added 3 commits July 24, 2019 14:20

Add single and bulk policy delete

5aa0505

Add policy execution

b9f88f4

Remove early destructuring of provider request responses

caec51e

jen-huang added release_note:enhancement v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI v7.4.0 labels Jul 24, 2019

jen-huang requested review from alisonelizabeth and cjcenizal July 24, 2019 22:36

jen-huang commented Jul 24, 2019

View reviewed changes

alisonelizabeth approved these changes Jul 25, 2019

View reviewed changes

jen-huang added 2 commits July 31, 2019 13:42

Merge remote-tracking branch 'upstream/master' into feature/slm-delete

af5e61b

Address PR feedback

7f93d84

jen-huang added 2 commits July 31, 2019 15:37

Adjust policy requests for useRequest changes

82a0ce8

Fix policy reload

6a6b963

jen-huang merged commit 6860574 into elastic:master Aug 1, 2019

jen-huang deleted the feature/slm-delete branch August 1, 2019 00:40

jen-huang mentioned this pull request Aug 1, 2019

[7.x] [SR] Delete and execute SLM policies (#41934) #42412

Merged

cjcenizal reviewed Aug 1, 2019

View reviewed changes

Conversation

jen-huang commented Jul 24, 2019 • edited by cjcenizal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Uh oh!

elasticmachine commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 24, 2019

💚 Build Succeeded

Uh oh!

alisonelizabeth 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!

elasticmachine commented Jul 31, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Aug 1, 2019

💚 Build Succeeded

Uh oh!

cjcenizal 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!

cjcenizal Aug 1, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jen-huang commented Jul 24, 2019 •

edited by cjcenizal

Loading

cjcenizal Aug 1, 2019 •

edited

Loading