Ask for promote when fully migrating a public app #326
Conversation
|
cc @ibolmo for you may be interested in having a look. |
There was a problem hiding this comment.
Cool thanks for the heads up! Quick look made me wonder if we want to scope outside the app, but I suppose passing down a "context" object with something like { app, answer } could be more busy. The advantage in doing it this way, though, will make it easier to refactor into individual functions.
src/commands/migrate.js
Outdated
| @@ -8,9 +15,45 @@ const migrate = (context, fromVersion, toVersion, optionalPercent = '100%') => { | |||
| return Promise.resolve(); | |||
| } | |||
| optionalPercent = parseInt(optionalPercent, 10); | |||
|
|
|||
| let app; | |||
There was a problem hiding this comment.
any reason to scope this outside?
There was a problem hiding this comment.
Thanks for the suggestion! Fixed in 032f11e with Promise.all.
There was a problem hiding this comment.
Looks good! my only other thought is that we're already using babel but not taking advantage of async // await anywhere, which seems silly. Could greatly simplify some of the promise chaining that happens in other commands (looking at you, build). Not related to this, but something I thought about while reading this.
src/commands/migrate.js
Outdated
| .then(linkedApp => { | ||
| app = linkedApp; | ||
| if ( | ||
| optionalPercent === 100 && |
There was a problem hiding this comment.
is there any value in setting this threshold a little lower? presumably moving 90% of users to a version means you want that version to be public too. I could even see a case for going lower, maybe 75%.
There was a problem hiding this comment.
It's hard to define such a threshold. You can always ask why this value and why not that value. So I think it's easier for us to just assume when a dev does a partial migration, they're testing. In addition, for private apps, the backend does implicit promotion (such as moving ZTs to the new version) only if it's a 100% migration. To make it symmetry on the public apps, shouldn't we only suggest for promotion when it's 100% migration?
There was a problem hiding this comment.
Yeah, that's fair! It's a pattern devs have some issues with, but maybe the consistency is worthwhile
src/commands/migrate.js
Outdated
| return answer; | ||
| }; | ||
|
|
||
| return utils.promiseDoWhile(action, stop); |
There was a problem hiding this comment.
not necessarily something to fix in this PR, but it would be worth abstracting input like this into a prompUser(question: string, acceptedAnswers?: string[])-type method. We could also use a library since it seems like we're doing a big of hack here with the promise loop. ¯\_(ツ)_/¯
src/commands/promote.js
Outdated
| @@ -9,7 +9,7 @@ const hasCancelled = answer => | |||
| const hasAccepted = answer => | |||
| answer.toLowerCase() === 'y' || answer.toLowerCase() === 'yes'; | |||
|
|
|||
| const promote = (context, version) => { | |||
| const promote = (context, version, printMigrateHint = true) => { | |||
|
@xavdid |
|
No need to wait! We're already running babel on I'm not sure why we haven't been writing stuff that way already. Maybe just habit? |
|
@xavdid I know we can start using |
|
that's fair! Since they work with one another it might be easier to introduce them as we go. i'm excited for that future though |
Addresses PDE-160.
It makes little sense to migrate 100% without a promotion beforehand. This PR adjusts the behavior of
zapier migrateso that when a dev does a 100% migration on a public app, we ask if the dev wants to promote that version first.As for private apps, the 100% migration should automatically do an implicit promotion for the devs. It's already being implemented in the backend, so we don't need to handle that here.