zapier / zapier-platform

The SDK for you to build an integration on Zapier
https://docs.zapier.com/platform
Other
345 stars 189 forks source link

Add the ability to force delete a integration version via the CLI #449

Open Celtech opened 2 years ago

Celtech commented 2 years ago

Assumptions

The following assumptions are made to give context behind this feature request, knowing these assumptions will help understand the need for this feature.

Current Behavior

Currently, we have access to the zapier delete:version x.x.x command. This command is very limited in its functionality right now, especially in the case of having users on an integration version. If you try to run this command on a version that has users you get the following message:

$ zapier delete:version 99.0.0
✔ Checking authentication & permissions
✖ Deleting version 99.0.0 of app "Test Integration"
 ›   Error: "https://zapier.com/api/platform/cli/apps/140XXX/versions/99.0.0" returned "403" saying "You cannot delete an app version with users"
 ›
 ›   re-run this command with `--debug` for more info

Why This Is a Problem

The above behavior is fine in many cases, especially for preventing accidental deletions but in the case of CI/CD especially in review and staging environments, this can be problematic. Let's assume a user makes a feature branch, then creates a merge request to develop. This branch would then get deployed to the review integration(let's say its version 1.1.1) for testing before merging. Now assume that someone goes and tests this integration, creates a zap, turns it on. The user finishes their testing, merges the branch, and calls it a day. Now in this scenario version 1.1.1 has 1 user, thus if we try to run the delete command we can't due to the error message in the above section. So what are our options with the current tools available?

As we can see none of the above options are really good options in the context of CI/CD, or environment automation, we should fix that and encourage it! Using CI/CD promotes well-tested integrations, fewer errors, easier and rapid deployments, and ease of clean up.

Desired Behavior

User Story

Acceptance Criteria

Important Considerations

Sample of How this Flag Would Be Used In a Pipeline

Stage Pipeline View: image DAG Pipeline View: image

xavdid commented 2 years ago

@Celtech First off, let me thank you for this truly superb feature request. You've really helped us understand the use case and context around the request.

Off the top of my head, the biggest issue is around what happens to the zaps after their app is gone. Live ones will error out and those opened in the editor will maybe blow up in an unexpected way. We can work around this, but it would take time.

We've had similar requests from devs wanting to clean up old apps that they're sure aren't being used for anything important. A potential compromise would be to only allow deletion w/ usage if all the zaps are in their account. Stuff could break, but it should be localized enough to not be an issue.

Ultimately, there's probably something we can go here to cater more to your advanced use case. We'll have to figure out approaches and schedule the work, but we'll look into it!

Celtech commented 2 years ago

@xavdid Hey there,

Thanks for taking the time to investigate and look into this guy!

That said I have another thought as to how this could function without unwanted behavior.

What if when the --force flag is applied to the delete command you trigger some sort of event that caused all running zaps to shut down, and possibly even remove them from the users account. In that case then the delete command could carry out just fine.

Another alternative would be to provide a new command such as zapier prune:version <version>. The prune command would shut down all running zaps much like above, allowing them to be deleted with the zapier delete:version <version> command which would also remedy this use case.

I can envision this working somewhat similar to the migrate command, maybe you can even reuse the logic. Logic would dictate that you have to query a list of all users running your integration in order to migrate each of them, in that case that means that we have that data exposed so we should be able to trigger some kind of even to turn off the zap similar to how turning off the switch in the UI would work. Please note I say all this without looking at the code. I'll dig into this a little later and see if I can't come up with some code samples to illustrate how I think this could work.

Let me know your thoughts!