Integration test cleanup workflow #138
Conversation
utils/cleanupResources.ts
Outdated
| (async () => { | ||
| const p = new pinecone.Pinecone(); | ||
|
|
||
| const collections = await p.listCollections(); | ||
| for (const collection of collections) { | ||
| await p.deleteCollection(collection.name); | ||
| } | ||
|
|
||
| const indexes = await p.listIndexes(); | ||
| for (const index of indexes) { | ||
| await p.deleteIndex(index.name); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
nit: Would it be useful to log anything out here if there were resources to clean up? Was thinking it might be nice to have some sort of indication of that either when running in the CLI or triggered via workflows.
There was a problem hiding this comment.
Great idea, I'll add some log output.
| var dotenv = require('dotenv'); | ||
| dotenv.config(); | ||
|
|
||
| for (const envVar of ['PINECONE_ENVIRONMENT', 'PINECONE_API_KEY']) { |
There was a problem hiding this comment.
question: This may be a minor issue, but if you run this locally outside CI where we're using secrets, I'd imagine this will just wipe things for whatever environment values you have configured, would that be an issue? Not sure if there's a good way around that as we don't want to embed the integration testing stuff in the repo itself.
There was a problem hiding this comment.
Yeah, it would run for whatever environment you are currently pointing at in your .env. Not ideal I suppose if you have a lot of test index data you're worried about accidentally deleting, but how likely are you to accidentally run npm run integration:test:cleanup from local? By wrapping this into a github actions workflow, we shouldn't ever really be running this locally.
There was a problem hiding this comment.
I suppose I could remove the dotenv invocation since that is only used locally. But I actually do want the ability to run this locally sometimes when developing new tests that throw off a lot of unintended indexes.
I think a more advanced version of this workflow would have us adopt a standardized naming scheme for indexes and collections (e.g. one that starts with a build number), and then trigger a cleanup job immediately after every CI run that deletes resources tied to that build number only. But that's more work / somewhat error prone. This quick and dirty approach gives most of the benefits.
There was a problem hiding this comment.
Yeah I ultimately think this is fine, it would be very unlikely somewhat would run the integration cleanup step by accident or without knowing what it does.
Problem
Integration tests attempt to cleanup after themselves with
afterEach, but it is possible for theafterEachdelete to fail or for someone to add a test that does not have proper cleanup steps. If we don't have a way to cleanup these orphaned resources, eventually all tests will fail due to quota exhaustion.For example, while trying to write tests for create collection I accidentally created a bunch of these abandoned resources because the afterEach call to deleteCollection was failing; collections can't be deleted while they are still being created.
Solution
Create a workflow that deletes all indexes and projects within the sdk-node-testing project.
Type of Change