Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Celtech opened this issue Nov 13, 2021 · 2 comments
Open

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

Celtech opened this issue Nov 13, 2021 · 2 comments

Comments

@Celtech
Copy link

Celtech commented Nov 13, 2021

Assumptions

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

  • The user has 3 integrations set up, these 3 integrations act as environments for testing your integration at different stages in the development life cycle
    • Production
    • Staging
    • Review
  • The staging and review environments are private
  • The production environment is public
  • All 3 environments are fully automated via CI/CD to version, build, deploy and clean up

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?

  • We can ask the user the turn off the version and re-run the job, but this kinda breaks the purpose of CI/CD and a user has to go back and interact with it again when they should never have to think about it
  • We can first migrate the users off the integration to another version then schedule the delete job for a later date. This would require a static version such as 0.0.0 which is never used for anything, that we can then migrate them to. But what happens if a feature branch removes an existing trigger? Well in that case our keys no longer match between 0.0.0 and 1.1.1, thus resulting in the migration failing with the error below. In fairness, we could always just build and upload our version to 0.0.0 before doing the migration, but who wants to build twice? On top of that, what happens if we have a migration happening to version 0.0.0, then another branch pushes to 0.0.0 while that's in progress? That sounds like errors waiting to happen!
$ zapier migrate 99.0.0 0.0.0                
✔ Checking authentication & permissions
✔ Starting migration from 99.0.0 to 0.0.0 for 100%
 ›   Error: "https://zapier.com/api/platform/cli/apps/140XXX/versions/99.0.0/migrate-to/0.0.0" returned "403" saying "v0.0.0 is missing the following keys (in triggers): 
"<trigger key here>". Hide them instead of removing them"
  • We could deprecate the version then schedule the delete job after the deprecation date. This has a big flaw though, the earliest deprecation date you can set is 2 weeks in the future. Beyond that integrations are limited to 50 versions, so assume we have a busy 2 weeks and make 26 merge requests. well since it takes 2 weeks to delete an integration that means we could have 52 integration versions. Consequently, that puts us over the limit of 50 so the last two versions will error when we try to push. This is a very realistic situation at our company!

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

  • As a developer, I can supply a flag(such as --force) to the zapier delete:version command to delete a version with users on it

Acceptance Criteria

  • Considering that I execute the zapier delete:version with the force delete flag, I should be prompted with a confirmation warning asking if I'm sure I want to delete this version. This should explain the consequences of doing so.
  • Considering that I execute the delete command with the force delete flag on an integration with no users, I should still be prompted with a confirmation warning asking the same as above.
  • Considering that I execute the delete command with the force delete flag on a public integration against the current promoted version, I should receive an error saying that you can't do that, the version should NOT be deleted

Important Considerations

  • What happens if we delete an integration with users on it? Will this cause anything to blow up?
  • What does it look like from the user's perspective that was using that integration version? Do we notify them? Does it show them any error messages? Does it break anything for them?
  • Should we disallow using the force flag on public integrations? If we did that means anything scary would only ever occur internally and in testing environments. It would also prevent mistakes on an integration that's customer-facing.

Sample of How this Flag Would Be Used In a Pipeline

Stage Pipeline View:
image
DAG Pipeline View:
image

@xavdid
Copy link
Contributor

xavdid commented Nov 16, 2021

@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
Copy link
Author

Celtech commented Nov 17, 2021

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants