Add --allow-updates and --allow-delete flags to deploy and release #6749
+420
−28
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gonzaloriestra
force-pushed
the
new-deploy-flags
branch
from
81e0dd4 to
76218bc
Compare
Contributor
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3592 tests passing in 1421 suites. Report generated by 🧪jest coverage report action from d5b37a1 |
gonzaloriestra
force-pushed
the
new-deploy-flags
branch
from
76218bc to
d5b37a1
Compare
Contributor
craigmichaelmartin
left a comment
craigmichaelmartin
left a comment
There was a problem hiding this comment.
Left a comment. I think generally in our documentation and command descriptions we should add copy advising against using --force and --allow-deletions . We still provide the sharp knifes, but there is certainly a safer approach: use --allow-updates in CI and seeing it's potential failing as a feature, giving you the mandate to manually deploy when doing potentially highly destructive actions. That is the approach our first party apps will use (EG checkout blocks which was the catalyst for this) and I think we should steer developers to it as well.
| if (!flags['no-release']) { | ||
| const hasAnyForceFlags = flags.force || flags['allow-updates'] || flags['allow-deletes'] | ||
| if (!hasAnyForceFlags) { | ||
| requiredNonTTYFlags.push('force') |
Contributor
There was a problem hiding this comment.
If a user is running shopify app deploy in non-TTY, I think our guidance should be to instruct them to add --allow-updates (not --force).
| const requiredNonTTYFlags: string[] = [] | ||
| const hasAnyForceFlags = flags.force || flags['allow-updates'] || flags['allow-deletes'] | ||
| if (!hasAnyForceFlags) { | ||
| requiredNonTTYFlags.push('force') |
Contributor
There was a problem hiding this comment.
Same here for @craigmichaelmartin's comment on the recommended flag.
| if (allowDeletes && !hasUpdates) return true | ||
|
|
||
| // If we're in non-TTY mode and there are changes that require confirmation, throw an error | ||
| if (!isTTY() && (hasDeletes || hasUpdates)) { |
Contributor
There was a problem hiding this comment.
I think we should be specific in this error message about what the required flag is here, based on what changed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Fixes https://github.com/shop/issues-develop/issues/21805
WHAT is this pull request doing?
Add new flags to app deploy and app release:
--allow-updateswould allow for adding and updating extensions and configuration without user prompt. It should be required for any non-interactive release.--allow-deleteswould allow for removing configuration and extensions without user prompt. It should be required for any non-interactive release which has removed configuration or extensions.Also,
deploy --no-releasedoes no longer require--force.How to test your changes?
Add/update/remove a extension or a config field like
embedded = falseunder[pos], and then try to deploy and release in different ways:CI=1--force--allow-updatesand/or--allow-deletesMeasuring impact
How do we know this change was effective? Please choose one:
Checklist