twilio-labs / serverless-toolkit

CLI tool to develop, debug and deploy Twilio Functions
https://www.twilio.com/docs/labs/serverless-toolkit
MIT License
114 stars 59 forks source link

Add cleanup command #311

Open dkundel opened 3 years ago

dkundel commented 3 years ago

Version 1.x of twilio-run at one point introduced a bug through a downstream dependency that would create additional unnecessary resources upon every deploy if the project had more than 50 Functions or more than 50 Assets.

Twilio runs an automated cleanup job every 30 days that removes unused Functions and Assets both the respective Versions as well as any unused Resources.

Since the same logic can be executed through the public APIs it would be useful to provide customers with the same functionality.

Logic

  1. Find all active Build instances across all Environments for a given Service
  2. Fetch all Function and Asset Resources
  3. Fetch all FunctionVersion and AssetVersion instances given the respective Functions and Assets
  4. Remove each FunctionVersion and AssetVersion that is not referenced in an active Build
  5. Remove all Function and Asset Resources that don't have an active FunctionVersion or AssetVersion respectively.

Considerations

Since this is a very destructive action we should provide a --dry-run option that will perform the same logic but will output all resources that would be deleted without actually deleting them.

Thought: Should we provide a prompt statement asking the user to confirm the deletion if --dry-run is not specified. To be friendly to CI/CD we should then also provide a --yes flag as well as respect the --force flag.

Format

$ twilio serverless:cleanup --help
Removes unused functions and assets from your service.

USAGE
  $ twilio serverless:cleanup

OPTIONS
  -c, --config=config                      Location of the config file. Absolute
                                           path or relative to current working
                                           directory (cwd)

  -l, --log-level=log-level                [default: info] Level of logging
                                           messages.

  -p, --profile=profile                    Shorthand identifier for your
                                           profile.

  -u, --username=username                  A specific API key or account SID to
                                           be used for deployment. Uses fields
                                           in .env otherwise

  --cwd=cwd                                Sets the directory of your existing
                                           Serverless project. Defaults to
                                           current directory

  --password=password                      A specific API secret or auth token
                                           for deployment. Uses fields from .env
                                           otherwise

  --service-sid=service-sid                SID of the Twilio Serverless Service
                                           to clean up

Possible Additional Features

310 is asking for a remove functionality to delete the service. One option is to have these two commands live side by side but that could be confusing to customers.

An alternative would be to add additional features to this command. The same functionality for example could be useful to delete a specific environment.

Option 1

Specific destructive flags such as --delete-service which will delete the entire specified service as well as --delete-environment which will delete an entire environment as well as perform the regular cleanup specified above.

If we go with this option the question would be if we should explicity require --service-sid or --environment as flags if either of the two --delete-* flags are being used or if we follow the same concept as any other command and try to infer these values.

Option 2

Dedicated subcommands. We could provide twilio serverless:cleanup:service and twilio serverless:cleanup:environment. The benefit would be that it's likely more discoverable than Option 1 but it would also raise the question if the base command should be twilio serverless:cleanup:unused instead.

Option 3

We have a remove command as a dedicated separate command. This could lead to confusion as mentioned above.

deshartman commented 3 years ago

@dkundel I think this is great idea and I like the approach. #310 was aimed at removing projects to avoid hitting the limit of functions. I realise that you could just execute the api remove command, but this is not obvious at all.

How about this approach?

My take is that if we are going to remove specific services or environments, it needs to mirror how we created them in the first place.

twilio serverless:init NAME twilio serverless:remove NAME twilio serverless:cleanup NAME

This removes the service from Functions, which aligns nicely with creating a new service regardless the environment in the service.

twilio serverless:deploy --environment=staging twilio serverless:remove --environment=staging twilio serverless:cleanup --environment=staging

This aligns with the process of creating an environment within a service. I think it would be good to have a remove and a clean option, because it is more obvious what you are trying to achieve; the 'verb' is used to deploy to an environment, so the command to clean up or remove should be similar.

This is not a 100% fit, since the NAME is most likely mandatory for remove and cleanup, with --environment as optional, but I think this is a nice clean way to represent the work flow to create, clean or remove.

I like the dry-run and --force approach, which makes it safe & CI/CD friendly.

Your approach is actually even better than the original #310 ask. I like it!

philnash commented 3 years ago

I do not think that cleanup and remove should be conflated into the same namespace or command.

cleanup is going to do a lot of work calculating what should and shouldn't remain amongst functions, function versions, assets, and asset versions. Ultimately, when cleanup is complete all active functions and assets in a service/environment should continue to work as before. Practically, cleanup is not destructive (though it is removing old versions/unused resources.

remove is destructive and should be separate to cleanup.

In the past I have used the regular CLI provided twilio api:serverless:v1:services:remove but that misses removing the serverless toolkit config in .twiliodeployinfo.

With the right documentation I think there should be separate commands:

twilio serverless:cleanup

With the command described as above under Format.

Then for removals:

twilio serverless:remove

And to remove just an environment (for convenience)

twilio serverless:remove --environment=staging

or possibly

twilio serverless:remove --environment-sid=xxx

Note: because twilio api:serverless:v1:services:remove exists, I do not think that twilio serverless:remove should have a --service-sid flag. It should be run within the context of a Serverless Toolkit controlled project and take the Service Sid from the config.

rmcsharry commented 1 year ago

I created a verify service.

Then I created a project using the CLI from a template. I deployed it. Now I cannot delete it.

Screenshot 2023-02-14 at 13 39 48

You can see the 2nd one I can delete because I did not use the CLI.

So how can I delete the 1st one if the CLI does not have such a command yet?

Also, I would expect the CLI to do whatever that delete button does.

philnash commented 1 year ago

@rmcsharry You can use the regular Twilio CLI to remove a service via the twilio api:serverless:v1:services:remove command.