Update cleanupResources workflow to check for deletionProtection #279
Conversation
| for (const collection of collectionList.collections) { | ||
| console.log(`Deleting collection ${collection.name}`); | ||
| await p.deleteCollection(collection.name); | ||
| if (collectionList.collections) { |
There was a problem hiding this comment.
Just added a quick check for undefined, since my IDE was throwing errors because collectionList.collections could be undefined.
.eslintrc.json
Outdated
| @@ -24,6 +24,7 @@ | |||
| "@typescript-eslint/ban-ts-comment": "off", | |||
| "@typescript-eslint/no-explicit-any": "off", | |||
| "@typescript-eslint/no-inferrable-types": "off", | |||
| "import/no-cycle": "error" | |||
| "import/no-cycle": "error", | |||
| "@typescript-eslint/no-var-requires": "off" | |||
There was a problem hiding this comment.
Long story w/this one, but essentially I added this because:
- Our
eslintconfig is set up to expectes6modules (which useimportfor imports) on the line"sourceType": "module". - Our
tsconfigfile, though, tells the compiler to use CJS syntax (which usesrequirefor imports) - So, linting complains about the
requirestatements in ourutils/files without the addition of this rule to oureslintconfig
There was a problem hiding this comment.
Deeper dive:
Q: Why doesn't my IDE complain about the files in dirs such as src/ using import instead of require, if tsconfig says we're supposed to be writing CJS?
- This is because we also have the line
"esModuleInterop": truein ourtsconfigfile. This allows us to use ESM syntax (import) in our Typescript files, even though the underlying module system is CJS.
Q: Why don't you just change the imports in the utils/ files to use import (ESM) instead of require (CJS) then?
- When I tried to do just that, I ran into the following error:
- This is because we run the files in
utils/with thenodecommand, andnodedoes not understandtypescriptores6(ESM) module syntax. By default,nodeexpectsCJSsyntax.
There was a problem hiding this comment.
Interesting, thanks for doing a deep dive on this. I see the IDE issues as well, but since we specifically lint src/ I think that's why this isn't generally an issue in CI and whatever.
I'd personally prefer something more specific more specific than disabling for the whole repo. I think we should probably look at cleaning some of these utility files up a bit.
There was a problem hiding this comment.
[ignore this -- made a ticket to address in a separate PR: https://app.asana.com/0/1203260648987893/1208186698556902/f]
| for (const index of response.indexes) { | ||
| console.log(`Deleting index ${index.name}`); | ||
| await p.deleteIndex(index.name); | ||
| if (response.indexes) { |
There was a problem hiding this comment.
(Same here wrt undefined)
| @@ -1,4 +1,7 @@ | |||
| var dotenv = require('dotenv'); | |||
| const dotenv = require('dotenv'); | |||
There was a problem hiding this comment.
Since this item doesn't change, I switched it to const on my IDE's advice :)
## Problem Currently, the `cleanupResources` script [fails](https://github.com/pinecone-io/pinecone-ts-client/actions/runs/10623637432/job/29452705669?pr=278) to delete some indexes that spin up during our integration test suite because some of these indexes have `deletionProtection` `enabled`. ## Solution Check for `deletionProtection` being `enabled`; if it is, set it to `disabled`. ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [ ] Non-code change (docs, etc) - [ ] None of the above: (explain here) ## Test Plan CI passes --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1208192096733434 - https://app.asana.com/0/0/1208186698556887
Problem
Currently, the
cleanupResourcesscript fails to delete some indexes that spin up during our integration test suite because some of these indexes havedeletionProtectionenabled.Solution
Check for
deletionProtectionbeingenabled; if it is, set it todisabled.Type of Change
Test Plan
CI passes