-
Notifications
You must be signed in to change notification settings - Fork 280
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
List the uncommitted files that the deploy command errors on #1944
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.
Good change!
One more suggestion. Since this feature is more focused for CI environments, can we try to find package-lock.json
(and variants) in the list of files and, if found, show a next step with "You should use npm ci
instead of npm i
" or similar?
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
2b0188e
to
b11c1dc
Compare
vi.mock('@shopify/cli-kit/node/output', async () => { | ||
return { | ||
outputContent: () => ({value: ''}), | ||
outputInfo: () => {}, | ||
outputWarn: () => {}, | ||
}; | ||
}); |
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 weren't being mocked in any way so I'm removing this.
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.
nice!
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, just a minor fix:
b11c1dc
to
fa9128e
Compare
Force Push Patch Notes
|
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.
🎩 With and without a linked storefront
Looks good to me
fa9128e
to
8e4d171
Compare
Force Push Patch Notes
|
WHY are these changes introduced?
Now that we're guiding merchants to use the
deploy
command a number of them are hitting an error that they haven't seen before:This is easy to debug locally because you can run
git status
to see what has changed but it's cumbersome when it's only happening in your CI environment. The most likely cause of this is that their package manager's lockfile is being updated during the dependency installation process but it's not easy to tell.WHAT is this pull request doing?
This PR updates the error message to include all of the file changes by capturing the changes with
git status -s
and adding them to the banner. For example, while working on this change locally I had the following output:Additionally if we find that there are lockfiles present in the diff we'll add an additional item in "next steps" to explain how you might be able to avoid those files changing during a CI deploy.
Checklist