-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
Automigration: Fail with non-zero exit code on migration failure #31923
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
Automigration: Fail with non-zero exit code on migration failure #31923
Conversation
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.
4 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
View your CI Pipeline Execution ↗ for commit c04cd64
☁️ Nx Cloud last updated this comment at |
f4fc217
to
0dac061
Compare
Hi @mrginglymus Thank you for your contribution! For me it makes a ton of sense to return with a non-zero exit code if automigrations are failing while running the |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
That would be my preference, I was just worried about changing default behaviours. Presumably no escape-hatch flag needed, unless applied as a general purpose flag to |
if (hasFailures) { | ||
throw new Error('Some migrations failed'); | ||
} |
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.
Let's throw the error at the end of the doAutomigrate
function instead, so that the dependencies can still be installed and the doctor command can run. If one automigration fails, it should not necessarily block others. Additionally, the automigrate
function can be called programmatically, and it should return the result of each automigration as it does so far. So by moving this piece of code to doAutomigrate
, we just influence how the automigrate
cli command behaves in the case of an automigration error.
6aa0dd2
to
c04cd64
Compare
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!
Automigration: Fail with non-zero exit code on migration failure (cherry picked from commit 3ac2fec)
Closes #31919
What I did
Added a
--fail
option to theautomigrate
command indicating that any failed migrations should cause a non-zero exit code.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/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 pull request has been released as version
0.0.0-pr-31923-sha-c04cd64d
. Try it out in a new sandbox by runningnpx [email protected] sandbox
or in an existing project withnpx [email protected] upgrade
.More information
0.0.0-pr-31923-sha-c04cd64d
fail-on-failed-migrations
c04cd64d
1751885888
)To request a new release of this pull request, mention the
@storybookjs/core
team.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=31923
Greptile Summary
Added a new
--fail
flag to the Storybook CLI's automigrate command that causes non-zero exit codes when migrations fail, enhancing CI/CD pipeline capabilities.fail
boolean option toAutofixOptionsFromCLI
interface incode/lib/cli-storybook/src/automigrate/types.ts
code/lib/cli-storybook/src/automigrate/index.ts
when migrations fail and fail flag is enabledcode/lib/cli-storybook/src/automigrate/index.test.ts
to verify both soft and hard failure modes--fail
(-f) option to CLI interface incode/lib/cli-storybook/src/bin/index.ts