PDE-2981: feature(cli) - Update "promote" and "migrate" to use new endpoint #480
Conversation
| percent, | ||
| name: 'migrate', | ||
| from_version: fromVersion, | ||
| to_version: toVersion, |
There was a problem hiding this comment.
Is this the right place to review map from?
https://github.com/zapier/zapier/blob/16c0ad7ce5dbb39f9802bd8b45c2305c35b77699/web/backend/zapier/app_platform/developer_cli/api/migration_resources.py
standielpls
changed the title
|
@eliangcs - not sure what testing this looks like but I can supply a screenshot of this working locally when pointing to zapier-staging.com |
| if (changelog) { | ||
| body.changelog = changelog; | ||
| body.job.changelog = changelog; |
There was a problem hiding this comment.
I know you were just following existing code, but you can move this above for the same effect:
const body = {
job: {
name: 'promote',
to_version: version,
changelog,
},
};Since it'll be undefined if it's missing, it just works
| @@ -15,6 +15,7 @@ class MigrateCommand extends BaseCommand { | |||
| const user = this.flags.user; | |||
| const fromVersion = this.args.fromVersion; | |||
| const toVersion = this.args.toVersion; | |||
| const email = this.args.email; | |||
There was a problem hiding this comment.
This won't quite work. If you look below at MigrateCommand.flags, we only define the user flag. I think if you provide --email, the CLI will error at you.
Because the name email vs user doesn't matter, I'd keep the existing flag name and just save it as email in the request body. That way the flags don't change for our users, but we send the right data to the API. Does that make sense?
There was a problem hiding this comment.
Yep, makes sense, good catch!
Because the name email vs user doesn't matter
Can you help me locate this for my own learning? I'm tracing where the endpoint builds the payload and find it set here but not sure where it will end up being used.
There was a problem hiding this comment.
https://cdn.zappy.app/eb7c153daa90542ec50c964bdc903dcb.png
nvm! I see now that user is email in this system!
| @@ -120,7 +126,7 @@ PromoteCommand.args = [ | |||
| { | |||
| name: 'version', | |||
| required: true, | |||
| description: 'The version you want promote.', | |||
| description: 'The version you want to promote.', | |||
| percent, | ||
| name: 'migrate', | ||
| from_version: fromVersion, | ||
| to_version: toVersion, |
There was a problem hiding this comment.
This is a great start! There are some things we'll want to fix before we ship, but we're most of the way there.
re: testing- we don't have great unit test coverage on CLI now, so as long as you've done thorough testing against staging/local, we're probably fine. Make sure to test all the args/flags! 😁
standielpls
force-pushed
the
pde-2981.promote-migrate-new-endpoint
branch
from
0e0b72f
to
3160a48
Compare
| @@ -58,8 +62,12 @@ class MigrateCommand extends BaseCommand { | |||
| `Starting migration from ${fromVersion} to ${toVersion} for ${percent}%` | |||
| ); | |||
| } | |||
| if (percent) { | |||
| body.job.percent_human = percent; | |||
There was a problem hiding this comment.
I believe this can also be included rigtht in the job above, since it's always defined (either from the default or from the user passing it explicitly)
standielpls
changed the title
Internally points the CLI
promoteandmigratecommand to use the new migration endpoint.These commands currently point to an endpoint that calls out to the new migration endpoint so this is work to remove that indirection.