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: Instruct the correct auto-migration command #26515

Merged
merged 6 commits into from
Mar 20, 2024

Conversation

ndelangen
Copy link
Member

Closes #26478

What I did

I re-used a solution also in used in the doctor command, to show the correct command-suggestion.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

I didn't test this myself manually.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@ndelangen ndelangen self-assigned this Mar 15, 2024
@ndelangen ndelangen changed the title fix https://github.com/storybookjs/storybook/issues/26478 CLI: instruct the correct auto-migration command Mar 15, 2024
@ndelangen ndelangen changed the title CLI: instruct the correct auto-migration command CLI: Instruct the correct auto-migration command Mar 15, 2024
Copy link

nx-cloud bot commented Mar 15, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a2582ba. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Comment on lines 63 to 65
const automigrateCommand = isPrerelease(versions.storybook)
? 'npx storybook@next automigrate'
: 'npx storybook@latest automigrate';
Copy link
Contributor

@valentinpalkovic valentinpalkovic Mar 17, 2024

Choose a reason for hiding this comment

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

If someone runs npx storybook@next upgrade and after the upgrade someone wants to rerun the automigrations, shouldn't we just tell the users to call npx storybook automigrate to run the local version? This makes the most sense to me. Because people might run a specific version of an upgrade like npx storybook@8.1.0 upgrade and then the above commands again do not fit, because the user wants to run the automigrate command from 8.1.0 and not from latest or next (If 8.1.0 doesn't match latest or next).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that that's a great idea, we should optimize for the common cases.
Before this PR, the suggested command to run was always with @next, this seems important to fix ASAP?

Because people might run a specific version of an upgrade like npx storybook@8.1.0 upgrade and then the above commands again do not fit

True, but it's not super common to happen?
There's possibly even more edge cases?

The code is also an exact copy of how we already this the exact thing somewhere else already:

const doctorCommand = isPrerelease(storybookVersion)
? 'npx storybook@next doctor'
: 'npx storybook@latest doctor';

Copy link
Member

@yannbf yannbf Mar 19, 2024

Choose a reason for hiding this comment

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

If a user runs a command to upgrade Storybook and it fails to upgrade, then if we tell them to run npx storybook <command> that will use the non-upgraded version, a version we might not want, right?

Copy link
Contributor

@valentinpalkovic valentinpalkovic Mar 20, 2024

Choose a reason for hiding this comment

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

@yannbf I might be mistaken, but the migration summary isn't shown when the upgrade fails. It should only tell the user that the automigrate CLI can be re-executed when automigrations are skipped, but the upgrade itself was successful. Otherwise, only running the automigrate command when the upgrade fails doesn't make much sense. You don't know whether the correct dependencies are already installed or in which phase the upgrade failed.

@ndelangen, Can you elaborate on your concerns about why it isn't a good idea? I am not convinced by the arguments yet. The argument that things traditionally were already done in a specific fashion doesn't tell anything about the quality of an idea or why it is good or bad.

We explicitly worked on the idea to enable people to upgrade to specific versions of Storybook. So, we have already identified this use case as pretty important, and we want to acknowledge it and not treat it as an edge-case.

If next is 8.4 and someone upgrades to 8.3 via npx storybook@8.3.0 upgrade it is a terrible idea to run automigrations from 8.4.0. I understand @yannbf argument, that 8.4 might have automigration improvement/fixes, which 8.3.0 doesn't have, on the other hand, the automigration scripts are versioned, and therefore they are coupled with the rest of Storybook's ecosystem in a particular point in time, opening the door for terrible bugs. Second, if Storybook 9 is published, and people upgrade from 7 to 8, should they run npx storybook@next automigrate or npx storybook@latest automigrate, which both point to a Storybook 9 version? I don't think that's what we want.

@ndelangen ndelangen added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Mar 20, 2024
@ndelangen ndelangen merged commit 1726622 into next Mar 20, 2024
54 of 61 checks passed
@ndelangen ndelangen deleted the norbert/fix-mention-next-in-cli branch March 20, 2024 19:19
@github-actions github-actions bot mentioned this pull request Mar 21, 2024
11 tasks
storybook-bot pushed a commit that referenced this pull request Mar 21, 2024
…n-cli

CLI: Instruct the correct auto-migration command
(cherry picked from commit 1726622)
@github-actions github-actions bot mentioned this pull request Mar 21, 2024
5 tasks
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:normal cli patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Instructions after automigrate to v8.0.0 would run migration to v8.1.0-alpha.1
3 participants