-
Notifications
You must be signed in to change notification settings - Fork 126
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
Do more cleanup on delete #180
Conversation
Ping, any update on this? :) @kamyarz-aws |
My suggestions |
Why do we want this to be different from how other data is treated? We remove the config, history, and (cloudwatch) dashboards without any extra confirmations
It does these two things, but I don't mind removing it from this PR:
When you delete a test, there are two kinds of files left over in S3 (that nothing refers to anymore): the test scenario's (and assets) and the test results. Test scenario's are easy to remove and have a bounded number of API calls that are needed to do that. Result files are unbounded. So to avoid an unknown runtime (and a bunch of list + delete calls), I decided to leave results in S3 when scenario's are deleted and instead delete any result older than 10 years.
Since versioning is enabled, deleting (or overwriting) files from S3 does not actually completely removes them. I added a lifecycle rule that removes versions that are not the current version after a year (we have backups for 35 days in dynamodb so this is a bunch longer).
That shouldn't be a problem
I can copy part of the explanation, if we agree that the approach here is acceptable
I can copy part of the explanation above
Can you look at my remark about this being different first? |
Hi, I went through the CR again, and we brought it up internally. I understand that you might not see value in your case in keeping certain assets in the s3 buckets. But specifically for the logs we think many clients may want to keep them in their buckets for longer period. We decided to hold this features until we collect more signals from our clients to come up with feature description and solution suitable for more clients. For now we think individual clients can manage their assets on their accord until we understand more clients needs and apply certain changes to resolve this. |
Description of changes:
This change will:
This seemed to use as a good compromise between things that are easy to delete (the test scripts) and things that would need an s3 list-objects (results, where we use a lifecycle instead)
I also included a script to look for resources for test that were already deleted
Checklist
Unit tests: They passed when run on our fork, but currently fail. Unit tests on the main branch also fail for me, so it seems unrelated to this change. I can rebase this branch after the main branch is fixed.
Breaking change: Debatable, it is a change in behaviour, but I don't think anyone would be relying on files sticking around for that long
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.