feat(cli): Introduce zapier canary command #861

pragmatic-zac · 2024-09-12T19:50:18Z

Adds a new command - zapier canary 🐦

This command enables users to divert a portion of traffic from one app version to a different app version for a specified amount of time.

Here's a gif of the command working...

First, we create a canary sending 10% of traffic from version 1.0.0 to 1.0.1 for 120 seconds
Then, we list all active canaries in a table
Next, we delete the active canary
Lastly, we re-list and see we've successfully deleted the canary

standielpls · 2024-09-12T19:59:38Z

Nice this looks so promising! And great turnaround
my quick initial feedback is i was thinking more on the lines of sub-commands like zapier env:get, zapier env:set (ref)

pragmatic-zac · 2024-09-12T20:19:27Z

Ah gotcha, yeah I like the subcommands better! I'll swap those in instead

pragmatic-zac · 2024-09-16T19:04:31Z

@standielpls just updated this to use subcommands so now we have canary:create, canary:list, and canary:delete. It's ready for a look whenever you have a chance 😃

standielpls · 2024-09-16T19:25:27Z

packages/cli/src/oclif/commands/canary/create.js

+    duration: flags.integer({char: 'd', description: 'Duration of the canary in seconds', required: true}),
+  },
+  opts: {
+    format: true


Why format: true? It also uses the -f flag so it will collide with versionFrom

Removed, thanks for catching that - it was a remnant from using a table for formatting

standielpls · 2024-09-16T19:48:38Z

packages/cli/src/oclif/commands/canary/create.js

+  }
+
+  validateDuration(duration) {
+    if (!duration || duration <= 30 || duration > 24 * 60 * 60) {


I think we want duration < 30

standielpls · 2024-09-16T20:19:03Z

packages/cli/src/oclif/commands/canary/create.js

+    versionFrom: flags.string({char: 'f', description: 'Version to route traffic from', required: true}),
+    versionTo: flags.string({char: 't', description: 'Version to canary traffic to', required: true}),


Let's move the from/to versions as "arguments" - these are things the command should act on. (migrate example)
Percent i think it's closer to a required argument as well, since this should be set by the user, I don't want to default to anything.
Flags are treated more like options, and are things we can use to alter the behaviour (like duration).

👍 I'd initially put them in flags because I thought it was little more descriptive, but I see what you mean. I moved everything to args

Do we want duration to be a flag though? Seems like a required arg to me, I'd prefer the user to be explicit about what they're wanting to do

standielpls · 2024-09-16T20:22:56Z

packages/cli/src/oclif/commands/canary/create.js

+    return activeCanaries.objects.find(c => c.from_version === versionFrom && c.to_version === versionTo) || null;
+  }
+
+  validateVersions(versionFrom, versionTo) {


Should check if versionFrom === versionTo as well

standielpls · 2024-09-16T20:29:20Z

packages/cli/src/oclif/commands/canary/create.js

+  }
+});
+CanaryCreateCommand.description =
+  'Create a new canary deployment, diverting a specified percentage of traffic from one version to another for a specified duration.'


I'd be a little bit more explicit here so it is clear to users what is going on.

Create a new canary deployment, diverting a specified percentage of traffic from one version to another for a specified duration. Only one canary can be active at the same time. You can run `zapier canary:list` to check. If you would like to create a new canary with different parameters, you can wait for the canary to finish, or delete it using `zapier canary:delete X Y`. Note: this is similar to `zapier migrate` but different in that this is temporary and will "revert" the changes once the specified duration is expired.

🤔 I want to say something about breaking changes as well (1.X.Y to 2.X.Y) and advise against it, or to block it from letting the user do it entirely, what do you think?

I incorporated your longer description, thank you!

Re: the breaking changes, I added a similar line from the migration command: **Only use this command to canary traffic between non-breaking versions!**

Here's what I was thinking. The endpoint for canary currently has a TODO to check for breaking changes before allowing a canary.

So I thought for now (for this v1) we could skip that check and simply warn the user not to do it. Then as part of v2, add the check to the API and do something more specific with the result here in the CLI. What do you think?

sounds good to me

standielpls · 2024-09-16T20:35:02Z

packages/cli/src/oclif/commands/canary/create.js

+    const existingCanary = await this.findExistingCanary(versionFrom, versionTo);
+    if (existingCanary) {
+      const secondsRemaining = existingCanary.until_timestamp - Math.floor(Date.now() / 1000);
+      this.log(`A canary deployment already exists from version ${versionFrom} to version ${versionTo}, there are ${secondsRemaining} seconds remaining`);


Maybe add a helper in there?

If you would like to stop this canary now, run `zapier canary:delete {versionFrom} {versionTo}`

standielpls · 2024-09-16T20:36:56Z

packages/cli/src/oclif/commands/canary/delete.js

+  }
+}
+
+CanaryDeleteCommand.flags = {


Similar to create, can we make these into arguments

standielpls · 2024-09-16T20:37:23Z

packages/cli/src/oclif/commands/canary/delete.js

+      return;
+    }
+
+    const confirmed = await this.confirm(`Are you sure you want to delete the canary from ${versionFrom} to ${versionTo}?`);


standielpls · 2024-09-16T20:44:00Z

packages/cli/src/oclif/commands/canary/delete.js

+
+  async findExistingCanary(versionFrom, versionTo) {
+    const activeCanaries = await listCanaries();
+    return activeCanaries.objects.find(c => c.from_version === versionFrom && c.to_version === versionTo) || null;


Do we need || null isn't .find() falsy if not found already ?

Nope, removed

standielpls · 2024-09-16T20:44:20Z

packages/cli/src/oclif/commands/canary/delete.js

+    return activeCanaries.objects.find(c => c.from_version === versionFrom && c.to_version === versionTo) || null;
+  }
+
+  validateVersions(versionFrom, versionTo) {


same, check if version from and to are the same

pragmatic-zac · 2024-09-17T21:18:14Z

packages/cli/src/oclif/commands/canary/create.js

+    if (activeCanaries.objects.length > 0) {
+      const existingCanary = activeCanaries.objects[0];
+      const secondsRemaining = existingCanary.until_timestamp - Math.floor(Date.now() / 1000);
+      this.log(`A canary deployment already exists from version ${existingCanary.from_version} to version ${existingCanary.to_version}, there are ${secondsRemaining} seconds remaining.


@standielpls one thing I'll call out here - since the API currently only allows one canary at a time, I've changed this code to block creation of a new one if any canary exists. Initially it checked for a canary matching the versionTo and versionFrom args, but I think this is simpler and offers less potential for confusion

So tldr, only one canary can exist at a time

I also don't super love the activeCanaries.objects[0] but the API returns the array right now, so 🤷

standielpls

NICE - just a couple more small things

standielpls · 2024-09-18T20:29:30Z

packages/cli/src/oclif/commands/canary/create.js

+  'Create a new canary deployment, diverting a specified percentage of traffic from one version to another for a specified duration. \n' +
+  '\n' +
+  'Only one canary can be active at the same time. You can run `zapier canary:list` to check. If you would like to create a new canary with different parameters, you can wait for the canary to finish, or delete it using `zapier canary:delete a.b.c x.y.z`. \n' +
+  '\n' +
+  'Note: this is similar to `zapier migrate` but different in that this is temporary and will "revert" the changes once the specified duration is expired. \n' +
+  '\n' +
+  '**Only use this command to canary traffic between non-breaking versions!**';


Can we use the same pattern as migrate and use string literals

CanaryCreateCommand.description = `Create a new canary deployment .... You can run \`zapier canary:list\` `

standielpls · 2024-09-18T20:34:27Z

packages/cli/src/oclif/commands/canary/create.js

-  'zapier canary:create --versionFrom=1.0.0 --versionTo=1.1.0 --percent=25 --duration=720',
-  'zapier canary:create -f 2.0.0 -t 2.1.0 -p 50 -d 300'
-]
+  'zapier canary:create 1.0.0 1.1.0 25 720',


Idk if this is a problem or not, but the percent and duration numbers can get mixed up easily. It'd be bad if they wanted 100s of canary and 30% traffic, but it was in the wrong order, creating a canary that is 100% traffic for 30s.

So I think it'd be nice to either

Show the summary upfront but ask for confirmation

turn percent/duration into options (so it can be passed in like -p 25, -d 30 for example)

I'm fine with either option

I moved percent and duration both back to options - either --percent or -p will work and same for duration

I feel better about this now. The potential for confusion was bothering me a bit too

standielpls

woooo!

pragmatic-zac added 6 commits September 10, 2024 16:10

Create base canary command

efa339a

Sketch in general structure of canary and API calls

f89747f

List canaries

66c2ccb

Cleanups and shuffle logic for better flow

368edd8

Extract individual functions for each subcommand

a1e6b78

Ensure canary exists before allowing deletion

f81f206

pragmatic-zac requested a review from a team as a code owner September 12, 2024 19:50

pragmatic-zac self-assigned this Sep 12, 2024

Check existence of canary before overwriting it

1803202

pragmatic-zac added 3 commits September 13, 2024 13:11

Check for existing canary on create

7e61b5c

Use subcommands

46b92bb

Small changes to percent for consistency

959de86

pragmatic-zac added 2 commits September 16, 2024 14:18

Add some examples

f5382a0

Use d for duration flag on create canary

202d967

standielpls reviewed Sep 16, 2024

View reviewed changes

pragmatic-zac added 3 commits September 17, 2024 14:52

Move from flags to args, validation and documentation improvements

ffcab37

Warning about breaking changes and helper for canary deletion

c3e3df0

Better messages on create

9385ea3

pragmatic-zac commented Sep 17, 2024

View reviewed changes

standielpls reviewed Sep 18, 2024

View reviewed changes

Move percent and duration to flags

1abb248

standielpls approved these changes Sep 19, 2024

View reviewed changes

pragmatic-zac merged commit 760595a into main Sep 19, 2024
14 checks passed

pragmatic-zac deleted the canary-v1 branch September 19, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): Introduce zapier canary command #861

feat(cli): Introduce zapier canary command #861

pragmatic-zac commented Sep 12, 2024

standielpls commented Sep 12, 2024

pragmatic-zac commented Sep 12, 2024

pragmatic-zac commented Sep 16, 2024

standielpls Sep 16, 2024

pragmatic-zac Sep 17, 2024

standielpls Sep 16, 2024

standielpls Sep 16, 2024

pragmatic-zac Sep 17, 2024

standielpls Sep 16, 2024

standielpls Sep 16, 2024

standielpls Sep 16, 2024

pragmatic-zac Sep 17, 2024

standielpls Sep 18, 2024

standielpls Sep 16, 2024

pragmatic-zac Sep 17, 2024

standielpls Sep 16, 2024

standielpls Sep 16, 2024

standielpls Sep 16, 2024

pragmatic-zac Sep 17, 2024

standielpls Sep 16, 2024

pragmatic-zac Sep 17, 2024

pragmatic-zac Sep 17, 2024

standielpls left a comment

standielpls Sep 18, 2024

standielpls Sep 18, 2024

pragmatic-zac Sep 18, 2024

standielpls left a comment

		versionFrom: flags.string({char: 'f', description: 'Version to route traffic from', required: true}),
		versionTo: flags.string({char: 't', description: 'Version to canary traffic to', required: true}),

feat(cli): Introduce zapier canary command #861

feat(cli): Introduce zapier canary command #861

Conversation

pragmatic-zac commented Sep 12, 2024

standielpls commented Sep 12, 2024

pragmatic-zac commented Sep 12, 2024

pragmatic-zac commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

standielpls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

standielpls left a comment

Choose a reason for hiding this comment