Skip to content
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

CLI: add automigration summary #20276

Merged
merged 6 commits into from
Dec 15, 2022
Merged

CLI: add automigration summary #20276

merged 6 commits into from
Dec 15, 2022

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Dec 14, 2022

Issue: #20222

What I did

Improved the automigration CLI command to provide a summary of what happened.

Successful migrations

image

No migrations applied

image

Migration with some success but also failures

image

Migration with only failures

image

Migration with multiple failures

In this example, the storybook version specifier is "next", which fails. That's a known bug and we'll fix it in a different PR.

image


Finally, an example with a full output (it's ugly, I know, but we can deal with making the package managers silent later):

image

How to test

You'll have to build the project, and alias the Storybook CLI binaries to then run in any project of your choosing.

alias sb='$HOME/open-source/storybook/code/lib/cli/bin/index.js'

# in whatever project:
sb upgrade --prerelease
  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@yannbf yannbf force-pushed the feat/automigration-summary branch from 8a53e38 to 7bdb4a0 Compare December 14, 2022 18:02
@yannbf yannbf self-assigned this Dec 14, 2022
@ndelangen
Copy link
Member

@yannbf amazing work!

I was hoping to pair with you on this. I think it's be great if we could pipe the full output (including package manager stdout) into a log file.

@ShaunEvening
Copy link
Contributor

@yannbf amazing work!

I was hoping to pair with you on this. I think it's be great if we could pipe the full output (including package manager stdout) into a log file.

A log file would be lovely :D Especially if it can declutter the CLI output a bit

@ShaunEvening
Copy link
Contributor

Multiple errors

Perhaps it's just the specific scenario, but with the errors being the same I would have expected the CLI to bail instead of trying each migration and sharing this.

@ndelangen
Copy link
Member

@integrayshaun I think @yannbf likely wanted to demo a bunch of things failing, and made them fail over the same reason.

In normal cases, this these would all fail over different reasons.
Our current strategy is to try to do as many migrations as we can, even if some of them fail.

Changing this would be a pretty big change, at a too late of a stage to change IMHO.

I understand where you're coming from, but getting 1 error at a time, can be just as infuriating and give the user a sensation that the errors will never stop.

By having all the migration ran, and show the errors all in 1 screen, we give the user a sense of how much is broken, and how much they likely need to fix.

If we 'hide' 4 errors, the user doesn't know if we're hiding 4 or 400, and might assume the worse. or assume it's only a single error, and be disappointed when it's "yet another one".

@ShaunEvening
Copy link
Contributor

image

The fact that we are separating this section with a horizontal line makes me feel like it should sit outside of the summary box... perhaps it's own underneath?

@yannbf
Copy link
Member Author

yannbf commented Dec 14, 2022

@yannbf amazing work!

I was hoping to pair with you on this. I think it's be great if we could pipe the full output (including package manager stdout) into a log file.

We could do that! Can you elaborate the use case though? And where the log file would be located?

@yannbf
Copy link
Member Author

yannbf commented Dec 14, 2022

image

The fact that we are separating this section with a horizontal line makes me feel like it should sit outside of the summary box... perhaps it's own underneath?

It could, but the idea was to have a single, final box at the end with the summary. There are a lot of boxes in the output already,it's possible, for instance, to have about 6-7 boxes in a single run if your storybook is quite outdated. The line was just to separate things a little, but we could do something else as well, or even remove the line and add a section in bold, like the "migrations that succeeded" and "migrations that failed" ones

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing work @yannbf! We really need Chromatic for our CLI though huh :) (just like we have for the Chromatic CLI ;))

@yannbf yannbf merged commit 67a0309 into next Dec 15, 2022
@yannbf yannbf deleted the feat/automigration-summary branch December 15, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants