[Snapshot + Restore] Add callout when restoring snapshot #95104

alisonelizabeth · 2021-03-22T19:56:45Z

This PR adds an info callout when a user enables global state while restoring a snapshot.

Fixes #83014
Fixes #92916

How to test

Specify a local repository when starting ES, e.g., yarn es snapshot --license trial -E path.repo=/tmp/es_backups
Using the Snapshot + Restore UI, create a local repository.
Create an index or add the sample Kibana data.
Create a SLM policy with the index create in step 3 selected and global state enabled (default).
Run the policy and verify successful snapshot.
Find the snapshot created in step 5 and click the "restore" action.
Enable "Restore global state" on the "Logistics" step and verify callout renders.

Screenshots

// Text change only

elasticmachine · 2021-03-23T12:35:44Z

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

alisonelizabeth · 2021-03-23T12:40:46Z

...components/restore_snapshot_form/steps/step_logistics/system_indices_overwritten_callout.tsx

+    >
+      <FormattedMessage
+        id="xpack.snapshotRestore.restoreForm.stepLogistics.systemIndicesCallOut.description"
+        defaultMessage="When this snapshot is restored, system indices will be overwritten with data from the snapshot. {learnMoreLink}"


The original recommended text also included: ... If this is not what you want, set include_global_state to false. This seems self-explanatory as the message only appears in the UI when global state is enabled, so I did not include it. Adding a comment here though in case anyone thinks it would be helpful to add back.

alisonelizabeth · 2021-03-23T12:42:36Z

/cc @gwbrown

jrodewig

LGTM. Copy seems clear. Thanks for this update @alisonelizabeth.

cjcenizal · 2021-03-23T15:50:18Z

.../public/application/components/restore_snapshot_form/steps/step_logistics/step_logistics.tsx

+             */}
+            {includeGlobalState && semverGt(version, '7.12.0') && (
+              <>
+                <EuiSpacer size="xs" />


This spacing looked a little tight to me in the screenshot. Might be worth checking with Michael?

@mdefazio would you mind taking a look? Screenshot in PR description.

Just based on looking at the screenshot, here's a rough guideline: (Spacing is tight, but we want to make sure there's more space between the button and the callout).

I've changed to spacing from xs --> s above the callout, and from l --> xl between the callout and the button. Let me know what you think. @mdefazio I'm not sure if you made a mistake when you said to "drop" 1-2 sizes in the screenshot, when in the original comment you said there should be more space between the button and callout. It's possible I misunderstood the screenshot though 😅 .

Another idea - we could simplify the callout and only have a title. Something like this (not committed):

My apologies for the poor direction 🤦 . I was simply trying to say that yes, there should be more space between the text and the callout, and it should be a bit less than the space between the button and the callout. So I didn't mean to say you had to change the space between the button and the callout (but the new screenshots look fine)

I like the callout with only the title. It's a bit less obtrusive and feels more straightforward.

afharo

Core CODEOWNER review LGTM

cjcenizal · 2021-03-23T17:59:25Z

...components/restore_snapshot_form/steps/step_logistics/system_indices_overwritten_callout.tsx

+  const { docLinks } = useCore();
+
+  return (
+    <EuiCallOut


Thinking about accessibility, I wonder if screen-readers announce the content of this callout when the user enables the global state toggle? If not then I think we should consider rendering the callout even when the toggle is disabled. If the callout is too prominent to show at all times, maybe we could just present it as standard description text.

Thanks for bringing this up! I've added an aria-live attribute so that it will be read by the screenreader when it is rendered.

If not then I think we should consider rendering the callout even when the toggle is disabled. If the callout is too prominent to show at all times, maybe we could just present it as standard description text.

If there is a general concern that the callout is too prominent, I've also included an example of just using the title in the callout: #95104 (comment). Adding it as a standard description text is a possibility as well.

…store_warning

kibanamachine · 2021-03-24T20:26:21Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`snapshotRestore`	167	176	+9

Async chunks

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

id	before	after	diff
`snapshotRestore`	455.5KB	468.6KB	+13.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	412.1KB	412.2KB	+100.0B

History

💔 Build #114987 failed a409aeb
💚 Build #114786 succeeded d733718
💚 Build #114531 succeeded cfe342c
💔 Build #114473 failed 2e7e930
💔 Build #114415 failed c616133

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

yuliacech · 2021-03-25T16:59:24Z

.../public/application/components/restore_snapshot_form/steps/step_logistics/step_logistics.tsx

+             * Note: Once we support features states in the UI, we will also need to add a check here for that
+             * See https://github.com/elastic/kibana/issues/95128 more details
+             */}
+            {includeGlobalState && semverGt(version, '7.12.0') && (


Thanks for adding the explanation about features states, it's very helpful to understand the code

yuliacech

Hi @alisonelizabeth, code changes LGTM! Tested locally and confirmed the callout is rendered correctly. Thanks a lot for adding tests as well.

…5452)

add callout when restoring snapshot

c616133

fix i18n

2e7e930

alisonelizabeth mentioned this pull request Mar 23, 2021

[Snapshot + Restore] Add support for "feature states" on restore #95128

Closed

remove unused import

cfe342c

alisonelizabeth marked this pull request as ready for review March 23, 2021 12:35

alisonelizabeth requested review from a team as code owners March 23, 2021 12:35

alisonelizabeth requested review from yuliacech and esdocs March 23, 2021 12:35

alisonelizabeth commented Mar 23, 2021

View reviewed changes

jrodewig approved these changes Mar 23, 2021

View reviewed changes

cjcenizal reviewed Mar 23, 2021

View reviewed changes

afharo approved these changes Mar 23, 2021

View reviewed changes

cjcenizal reviewed Mar 23, 2021

View reviewed changes

alisonelizabeth added 3 commits March 23, 2021 22:03

address review feedback

d733718

Merge branch 'master' of https://github.com/elastic/kibana into sr/re…

01aff28

…store_warning

update callout

a409aeb

alisonelizabeth removed the request for review from esdocs March 24, 2021 17:31

update translations

990c414

yuliacech reviewed Mar 25, 2021

View reviewed changes

yuliacech approved these changes Mar 25, 2021

View reviewed changes

alisonelizabeth merged commit 43e3d55 into elastic:master Mar 25, 2021

alisonelizabeth deleted the sr/restore_warning branch March 25, 2021 18:42

alisonelizabeth mentioned this pull request Mar 25, 2021

[7.x] [Snapshot + Restore] Add callout when restoring snapshot (#95104) #95452

Merged

alisonelizabeth added a commit that referenced this pull request Mar 25, 2021

[Snapshot + Restore] Add callout when restoring snapshot (#95104) (#9…

427f4e7

…5452)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Snapshot + Restore] Add callout when restoring snapshot #95104

[Snapshot + Restore] Add callout when restoring snapshot #95104

alisonelizabeth commented Mar 22, 2021 •

edited

Loading

elasticmachine commented Mar 23, 2021

alisonelizabeth Mar 23, 2021

alisonelizabeth commented Mar 23, 2021

jrodewig left a comment

cjcenizal Mar 23, 2021

alisonelizabeth Mar 23, 2021

mdefazio Mar 23, 2021

alisonelizabeth Mar 24, 2021

mdefazio Mar 24, 2021

afharo left a comment

cjcenizal Mar 23, 2021

alisonelizabeth Mar 24, 2021

kibanamachine commented Mar 24, 2021

yuliacech Mar 25, 2021

yuliacech left a comment

[Snapshot + Restore] Add callout when restoring snapshot #95104

[Snapshot + Restore] Add callout when restoring snapshot #95104

Conversation

alisonelizabeth commented Mar 22, 2021 • edited Loading

How to test

Screenshots

elasticmachine commented Mar 23, 2021

Choose a reason for hiding this comment

alisonelizabeth commented Mar 23, 2021

jrodewig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 24, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Choose a reason for hiding this comment

yuliacech left a comment

Choose a reason for hiding this comment

alisonelizabeth commented Mar 22, 2021 •

edited

Loading