-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Upgrade Assistant] Overview page redesign #106521
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
Conversation
f15e91d to
470150b
Compare
|
@elasticmachine merge upstream |
cjcenizal
left a comment
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.
Great work @sabarasaba! I'd like to get this merged ASAP to unblock other work on the Overview page, e.g. the step for reminding the users to back up their data. I left a number of comments, but I'd be happy to see them addressed after merging. I think it's OK to expedite this merge since this work is behind a feature flag.
x-pack/plugins/upgrade_assistant/public/application/components/overview/overview.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/public/application/components/overview/overview.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/public/application/components/overview/overview.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/__jest__/client_integration/helpers/overview.helpers.ts
Outdated
Show resolved
Hide resolved
...upgrade_assistant/__jest__/client_integration/overview/indentify_step/identify_step.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/server/lib/es_deprecation_logging_apis.ts
Outdated
Show resolved
Hide resolved
...de_assistant/public/application/components/overview/identify_step/use_deprecation_logging.ts
Outdated
Show resolved
Hide resolved
...de_assistant/public/application/components/overview/identify_step/use_deprecation_logging.ts
Outdated
Show resolved
Hide resolved
...s/upgrade_assistant/public/application/components/overview/review_step/es_stats/es_stats.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/public/application/lib/utils.ts
Outdated
Show resolved
Hide resolved
|
@sabarasaba Since this feature is behind a flag I think we can skip release notes for it. |
|
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
afharo
left a comment
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.
Codeowner review: Core changes LGTM! Only adding new doclinks
|
@elasticmachine merge upstream |
alisonelizabeth
left a comment
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.
Awesome work @sabarasaba! This is looking really great.
Most of my review comments are fairly minor. However, I did find one bug that I'd like to see addressed before approving. It looks like the label for the stats box was updated to show "warning" deprecations, but the actual code that determined this number was not. In other words, the count is still showing total deprecations even though the UI label makes it appear as though it's representative of warning. I believe this affects both ES and Kibana deprecations.
A few other comments:
- The Discover and Observability links aren't working for me 🤔. I'm going to bootstrap again and see if it's a problem on my end. Will report back if I can reproduce consistently.
- Can you remind me the name of the config to "fake" being on cloud so I can test step 3 when cloud is enabled?
- [nit] It was a little hard to tell that part of the error was clickable (only knew to click it after looking through the code). /cc @dborodyansky
x-pack/plugins/upgrade_assistant/__jest__/client_integration/helpers/services_mock.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/__jest__/client_integration/helpers/services_mock.ts
Outdated
Show resolved
Hide resolved
.../upgrade_assistant/__jest__/client_integration/overview/identify_step/identify_step.test.tsx
Outdated
Show resolved
Hide resolved
...gins/upgrade_assistant/__jest__/client_integration/overview/review_step/review_step.test.tsx
Outdated
Show resolved
Hide resolved
...ugins/upgrade_assistant/public/application/components/overview/upgrade_step/upgrade_step.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/server/lib/telemetry/usage_collector.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/server/routes/deprecation_logging.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/upgrade_assistant/server/routes/deprecation_logging.test.ts
Outdated
Show resolved
Hide resolved
|
@dborodyansky / @debadair I'd like to get your feedback on something. I'm a little concerned on how and where we use the term "warning".
I may be overthinking this 😄 , but do either of you think this may cause confusion to a user? |
|
Also - @sabarasaba I believe the merge conflicts are due to #105998 being merged. Let me know if you need help addressing this! |
@alisonelizabeth, In my opinion, this doesn't flag much potential user confusion but eager to learn from other perspectives. We consistently label as "warnings", and "critical (warning)" is an additional signifier to promote attention. We don't explicitly label "critical warnings" instead of just "critical" on the cards for brevity, but I believe this comes across as implied. Open to input as always. |
Looks like there might be an issue here. Took a quick look, and it seems the problem is It doesn't look like |
dborodyansky
left a comment
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.
@sabarasaba Layout looks great! Nice responsive adjustments for narrower viewports as well 👍 . Small suggestion on narrow view would be to wrap the Documentation button lower than the header content.
That said, if the general "Documentation" link is still warranted, we may want to consider stacking docs links together as such:
Discover and Observability links do not work for me. Also unsure how to test conditional states such as:
- Cloud version of step 3
- Error state for deprecation logging switch.
Please let me know if I am missing something to test above scenarios. Great work!
|
Thanks for reviewing @alisonelizabeth! I've addressed most of the comments, left a few more screenshots and provided steps for testing cloud related stuff. |
|
Thanks for reviewing @dborodyansky! I agree with the cloud link being a bit too spaced out when the viewport is less than 770px, and since I don't think any of our apps is much usable below the 1000px mark I think we can defer this for later if you are ok with that? I've added steps for testing the cloud related step and for being able to see an error message for the toggle. |
e1a6202 to
bf3f1bb
Compare
alisonelizabeth
left a comment
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.
Latest LGTM! Confirmed Discover/Observability links are now working correctly and the deprecation stats now reflect the correct numbers.
Found a small typo when retesting and left a nit about how to add a class to EuiTooltip.
...ugins/upgrade_assistant/public/application/components/overview/upgrade_step/upgrade_step.tsx
Outdated
Show resolved
Hide resolved
...istant/public/application/components/overview/review_logs_step/kibana_stats/kibana_stats.tsx
Outdated
Show resolved
Hide resolved
dborodyansky
left a comment
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.
Thank you for the additional steps @sabarasaba. Agree we could defer additional responsive enhancements. I added a small spacing suggestion for the deprecation logging switch and error. Great work!
...overview/fix_deprecation_logs_step/deprecation_logging_toggle/deprecation_logging_toggle.tsx
Outdated
Show resolved
Hide resolved
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @sabarasaba |






Fixes: #79208
Summary
Improve Upgrade Assistant overview page with more comprehensive instructions, documentation links and add links to observability and discovery to allow users to better understand their deprecation logs.
Upper limit for review logs step
Error for review logs step stats panel
Just one stat shown for review logs step stats panel
How to test
xpack.upgrade_assistant.readonly: falsein yourkibana.ymlyarn es snapshot --license=trialyarn startHow to test cloud related changes
When running Kibana locally add following values to
kibana.ymlfile to simulate Cloud environmentHow to test toggle error messages
deprecation_loggingwithfake_deprecation_loggingit should return a 404 error.deprecation_loggingwithfake_deprecation_loggingit should return a 404 error.