-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Upgrade Assistant] Update UX for 7.17 #119798
Conversation
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I tested locally and works as expected.
I left a few note around using "to prepare for the upgrade" (which is the terminology our docs use).
...upgrade_assistant/__jest__/client_integration/overview/migrate_system_indices/flyout.test.ts
Outdated
Show resolved
Hide resolved
.../upgrade_assistant/public/application/components/es_deprecation_logs/es_deprecation_logs.tsx
Outdated
Show resolved
Hide resolved
@@ -38,7 +38,7 @@ const i18nTexts = { | |||
apiCompatibilityNoteBody: (docLink: string) => ( | |||
<FormattedMessage | |||
id="xpack.upgradeAssistant.overview.apiCompatibilityNoteBody" | |||
defaultMessage="We recommend you resolve all deprecation issues before upgrading. If needed, you can apply API compatibility headers to requests that use deprecated features. {learnMoreLink}." | |||
defaultMessage="You can start resolving deprecation issues before upgrading to the next major version of Elastic. If needed, you can apply API compatibility headers to requests that use deprecated features. {learnMoreLink}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of:
"You can start resolving deprecation issues to prepare the upgrade to the next major..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave this largely as is and just append "to the next major version of Elastic." We're trying to present the API compatibility headers as a less preferred safety mechanism. It doesn't really work without "We recommend..."
defaultMessage="You can start resolving deprecation issues before upgrading to the next major version of Elastic. If needed, you can apply API compatibility headers to requests that use deprecated features. {learnMoreLink}." | |
defaultMessage="We recommend you resolve all deprecation issues before upgrading to the next major version of Elastic. If needed, you can apply API compatibility headers to requests that use deprecated features. {learnMoreLink}." |
.../plugins/upgrade_assistant/public/application/components/es_deprecations/es_deprecations.tsx
Outdated
Show resolved
Hide resolved
@@ -30,7 +30,8 @@ const i18nTexts = { | |||
defaultMessage: 'Kibana deprecation issues', | |||
}), | |||
pageDescription: i18n.translate('xpack.upgradeAssistant.kibanaDeprecations.pageDescription', { | |||
defaultMessage: 'Resolve all critical issues before upgrading.', | |||
defaultMessage: | |||
'Resolve all critical issues before upgrading to the next major version of Elastic.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, but let's see what @debadair thinks, I'd put "to prepare the upgrade to"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think "to prepare" is necessary. I think it's understood that you're preparing to upgrade.
...upgrade_assistant/public/application/components/overview/fix_issues_step/fix_issues_step.tsx
Outdated
Show resolved
Hide resolved
...ugins/upgrade_assistant/public/application/components/overview/upgrade_step/upgrade_step.tsx
Outdated
Show resolved
Hide resolved
Thanks for having a look @sebelga! I've addressed your feedback about the tests and docs link. I'll defer on the copy changes to the docs team 🚀 |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these changes @sabarasaba. I noted some grammatical issues with a few bits of the copy. I also don't think we should go overboard with "to prepare," etc. here as long as it's clear that the steps are related to a major version upgrade.
My only large piece of feedback is related to step 3. I think we should update this step to advise users to come back to the page when they're ready to upgrade to 8.x. The current wording is kind of circular.
.../upgrade_assistant/public/application/components/es_deprecation_logs/es_deprecation_logs.tsx
Outdated
Show resolved
Hide resolved
@@ -38,7 +38,7 @@ const i18nTexts = { | |||
apiCompatibilityNoteBody: (docLink: string) => ( | |||
<FormattedMessage | |||
id="xpack.upgradeAssistant.overview.apiCompatibilityNoteBody" | |||
defaultMessage="We recommend you resolve all deprecation issues before upgrading. If needed, you can apply API compatibility headers to requests that use deprecated features. {learnMoreLink}." | |||
defaultMessage="You can start resolving deprecation issues before upgrading to the next major version of Elastic. If needed, you can apply API compatibility headers to requests that use deprecated features. {learnMoreLink}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave this largely as is and just append "to the next major version of Elastic." We're trying to present the API compatibility headers as a less preferred safety mechanism. It doesn't really work without "We recommend..."
defaultMessage="You can start resolving deprecation issues before upgrading to the next major version of Elastic. If needed, you can apply API compatibility headers to requests that use deprecated features. {learnMoreLink}." | |
defaultMessage="We recommend you resolve all deprecation issues before upgrading to the next major version of Elastic. If needed, you can apply API compatibility headers to requests that use deprecated features. {learnMoreLink}." |
.../plugins/upgrade_assistant/public/application/components/es_deprecations/es_deprecations.tsx
Outdated
Show resolved
Hide resolved
@@ -30,7 +30,8 @@ const i18nTexts = { | |||
defaultMessage: 'Kibana deprecation issues', | |||
}), | |||
pageDescription: i18n.translate('xpack.upgradeAssistant.kibanaDeprecations.pageDescription', { | |||
defaultMessage: 'Resolve all critical issues before upgrading.', | |||
defaultMessage: | |||
'Resolve all critical issues before upgrading to the next major version of Elastic.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think "to prepare" is necessary. I think it's understood that you're preparing to upgrade.
...upgrade_assistant/public/application/components/overview/fix_issues_step/fix_issues_step.tsx
Outdated
Show resolved
Hide resolved
...ugins/upgrade_assistant/public/application/components/overview/upgrade_step/upgrade_step.tsx
Outdated
Show resolved
Hide resolved
...ugins/upgrade_assistant/public/application/components/overview/upgrade_step/upgrade_step.tsx
Outdated
Show resolved
Hide resolved
@sabarasaba I think this is going to |
@sebelga Correct, AFAIK we want this to ship with 7.16.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💥 These changes LGTM. My only concerns are addressed by James's suggestions. Thanks for the fast turnaround on this! We will cut a new BC on Thursday, so let's aim to merge this by Wednesday, 5PM Pacific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test locally, but this LGTM based on the screenshots and code. There was one suggestion that may have been overlooked, but it's not blocking either way: #119798 (comment)
Thanks for the fast work on this @sabarasaba! 🚀
Co-authored-by: James Rodewig <[email protected]>
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @sabarasaba |
This reverts commit 34dcc1e.
This reverts commit 34dcc1e.
This reverts commit 34dcc1e.
Fixes #119738
Summary
Given that we'll have another minor update before the next major, we need to tweak the UX of Upgrade Assistant to give the right message to our users. In order to achieve that, there is a few changes we'll have to do to the copy of the page to make it clearer that this is a preparation for the next major version:
Also, a few UI elements were hidden from the UI:
About system indices migration
Initially the Upgrade Assistant in 7.16 was designed specifically for upgrading to the next major. But with the addition of the 7.17 release, we decided to only leave the necessary tools to the user to assist him with the upcoming minor upgrade. We then ended up removing the system indices migration from the UI since it was designed to assist users into major upgrades and not minors in an effort to align it more with some of the thoughts explained with #117220.
With these changes we expect users to upgrade to 7.17 before actually migrating to the next major, as we always guide them to do. Note that I removed the entry point for system indices management from the code and skipped the tests, the code wont get into the bundle since its not required anywhere.
Screenshots
Overview of changes