Implement support for `DeletionProtection` #50

austin-denoble · 2024-07-24T03:55:08Z

Problem

Our other SDKs support the deletion protection feature with the latest major releases tied to the 2024-07 API version. We need to implement deletion protection in the Go SDK.

Solution

Add new DeletionProtection enum type to models.go.
Update CreatePodIndexRequest, CreateServerlessIndexRequest, and Index to include DeletionProtection.
Update ConfigureIndex to support a ConfigureIndexParams struct argument. This aligns more closely with our other SDKs, and makes it easier to pass configurations of arguments without needing to explicitly pass nil.

Type of Change

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

Test serverless and pod creation functions to make sure DeletionProtection defaults to disabled, and vice versa when provided. Test ConfigureIndex to make sure configuring an index without passing DeletionProtection doesn't override things.

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1207826038714269

aulorbe · 2024-07-25T23:14:42Z

pinecone/client.go

@@ -548,6 +552,7 @@ func (c *Client) CreatePodIndex(ctx context.Context, in *CreatePodIndexRequest)
 //     and consist only of lower case alphanumeric characters or '-'.
 //   - Dimension: The [dimensionality] of the vectors to be inserted in the Index.
 //   - Metric: The metric used to measure the [similarity] between vectors ('euclidean', 'cosine', or 'dotproduct').
+//   - DeletionProtection:


Is this supposed to be filled in with a definition?

Yup, whoops missed updating the struct comments. Thanks!

aulorbe · 2024-07-25T23:15:53Z

pinecone/client.go

+// ConfigureIndexParams contains parameters for configuring an index. For both pod-based
+// and serverless indexes you can configure the DeletionProtection status for an index.
+// For pod-based indexes you can also configure the number of Replicas and the PodType.
+// Each of the fields are optional, but at least one field must be set.


When you say "each of the fields" here, do you mean btwn replicas and podtype or btwn those 2 + also deletion protection? It's not entirely clear to me from how this sentence is worded.

Between all 3, you can still pass Replicas or PodType to the server, it'll just respond with an error if it's a serverless index.

We enforce sending at least one argument in the struct on the client-side.

aulorbe · 2024-07-25T23:16:20Z

pinecone/client.go

+//   - Replicas: (Optional) The number of replicas to scale the index to. This is capped by
+//     the maximum number of replicas allowed in your Pinecone project. To configure this number,
+//     go to [app.pinecone.io], select your project, and configure the maximum number of pods.
+//   - DeletionProtection: (Optional) // DeletionProtection determines whether [deletion protection]


Should the sentence here start with //?

Might be nice to also hyperlink this to documentation

Ah I meant to add it as a link in the bottom with the references. Nice catch.

aulorbe · 2024-07-25T23:17:35Z

pinecone/client.go

 // ConfigureIndex is used to [scale a pods-based index] up or down by changing the size of the pods or the number of
-// replicas.
+// replicas, or to enable and disable deletion protection for an index.


Sorry for being behind the times, but so now you can call ConfigureIndex on a serverless index?

Should we add a test that tries to call ConfigureIndex on a serverless index, but passes in replicas and pods (which obvi aren't things on serverless indexes), and assert a failure? I think we should!

Could also just assert that we can call ConfigureIndex on a serverless index while only passing in DeletionProtection and everything goes well.

Yes, let me add a couple tests.

aulorbe · 2024-07-25T23:18:58Z

pinecone/client.go

-// [app.pinecone.io]: https://app.pinecone.io
-func (c *Client) ConfigureIndex(ctx context.Context, name string, podType *string,
-	replicas *int32) (*Index, error) {
+func (c *Client) ConfigureIndex(ctx context.Context, name string, in *ConfigureIndexParams) (*Index, error) {


I like passing in a params struct here, nice thinking

aulorbe · 2024-07-25T23:20:28Z

pinecone/client.go

@@ -1153,13 +1209,16 @@ func toIndex(idx *control.IndexModel) *Index {
 		Ready: idx.Status.Ready,
 		State: IndexStatusState(idx.Status.State),
 	}
+	deletionProtection := derefOrDefault(idx.DeletionProtection, "")


Should this default to control.DISABLED instead of ""? I thought it could only ever be the enum vals?

aulorbe · 2024-07-25T23:24:42Z

pinecone/client_test.go

-	_, err = ts.client.ConfigureIndex(context.Background(), name, nil, nil)
-	require.ErrorContainsf(ts.T(), err, "must specify either podType or replicas", err.Error())
+	_, err = ts.client.ConfigureIndex(context.Background(), name, &ConfigureIndexParams{})
+	require.ErrorContainsf(ts.T(), err, "must specify either PodType, Replicas, or DeletionProtection", err.Error())


I might just be accidentally overlooking it here, but should there also be a test simply testing deletion protection on its own? Like, if you pass in control.ENABLED, and then try to delete an index, you should not be able to, right?

aulorbe

Generally, this looks good to me! I like making the params into a struct and then passing that.

But I think you might want to add more tests for deletion protection. I see just 1 addition to a test that's already there (the one where you don't pass replicas, podType, or deletionProtection in), but I don't think that's enough... is it possible to add more, like I say here? (LMK if I'm just missing something and there are more tests!)

And more testing thoughts here #50 (comment)

…update doc comments for updated functions and types

…tion protection

austin-denoble · 2024-07-26T23:54:34Z

But I think you might want to add more tests for deletion protection. I see just 1 addition to a test that's already there (the one where you don't pass replicas, podType, or deletionProtection in), but I don't think that's enough... is it possible to add more, like I say here? (LMK if I'm just missing something and there are more tests!)

And more testing thoughts here #50 (comment)

Good call, tests added. For now they're testing pod and serverless explicitly.

aulorbe · 2024-07-27T00:29:28Z

pinecone/client.go

@@ -1324,6 +1394,14 @@ func valueOrFallback[T comparable](value, fallback T) T {
 	}
 }

+func pointerOrNil[T comparable](value T) *T {


frickin' Go 🙄

aulorbe · 2024-07-27T00:29:48Z

pinecone/client_test.go

+	err = ts.client.DeleteIndex(context.Background(), name)
+	require.ErrorContainsf(ts.T(), err, "failed to delete index: Deletion protection is enabled for this index", err.Error())
+
+	// make sure configuring the index without specifying DeletionProtection maintains "enabled" state


(I actually didn't know it worked like this, super cool)

aulorbe

looks great!

austin-denoble force-pushed the adenoble/implement-deletion-protection branch from 9d5e110 to 45b6a71 Compare July 24, 2024 21:06

austin-denoble requested review from aulorbe, haruska and jhamon July 25, 2024 18:51

haruska approved these changes Jul 25, 2024

View reviewed changes

aulorbe reviewed Jul 25, 2024

View reviewed changes

aulorbe suggested changes Jul 25, 2024

View reviewed changes

austin-denoble added 8 commits July 26, 2024 19:46

add support for DeletionProtection to Index models and operations

d6d50dd

remove print statements, add check for empty ConfigureIndexParams values

16b1e60

clean up test file

fc13c64

print collection errors in CI

4af5ef6

update the other one this time

803aecd

add custom DeletionProtection type and use that in index operations, …

d1658bc

…update doc comments for updated functions and types

update Index type to use custom DeletionProtection

5c9fc64

review feedback, fix doc comments, add two integration tests for dele…

a1a119b

…tion protection

austin-denoble force-pushed the adenoble/implement-deletion-protection branch from 6677b88 to a1a119b Compare July 26, 2024 23:51

aulorbe reviewed Jul 27, 2024

View reviewed changes

aulorbe approved these changes Jul 27, 2024

View reviewed changes

austin-denoble merged commit 08d2c76 into main Jul 27, 2024
3 checks passed

austin-denoble deleted the adenoble/implement-deletion-protection branch July 27, 2024 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for `DeletionProtection` #50

Implement support for `DeletionProtection` #50

austin-denoble commented Jul 24, 2024 •

edited

Loading

aulorbe Jul 25, 2024 •

edited

Loading

austin-denoble Jul 26, 2024

aulorbe Jul 25, 2024 •

edited

Loading

austin-denoble Jul 26, 2024

aulorbe Jul 25, 2024

aulorbe Jul 25, 2024

austin-denoble Jul 26, 2024

aulorbe Jul 25, 2024

aulorbe Jul 25, 2024 •

edited

Loading

austin-denoble Jul 26, 2024

aulorbe Jul 25, 2024

aulorbe Jul 25, 2024 •

edited

Loading

aulorbe Jul 25, 2024

aulorbe left a comment •

edited

Loading

austin-denoble commented Jul 26, 2024

aulorbe Jul 27, 2024

aulorbe Jul 27, 2024

aulorbe Jul 27, 2024

aulorbe left a comment

Implement support for DeletionProtection #50

Implement support for DeletionProtection #50

Conversation

austin-denoble commented Jul 24, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

aulorbe Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe left a comment • edited Loading

Choose a reason for hiding this comment

austin-denoble commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe left a comment

Choose a reason for hiding this comment

Implement support for `DeletionProtection` #50

Implement support for `DeletionProtection` #50

austin-denoble commented Jul 24, 2024 •

edited

Loading

aulorbe Jul 25, 2024 •

edited

Loading

aulorbe Jul 25, 2024 •

edited

Loading

aulorbe Jul 25, 2024 •

edited

Loading

aulorbe Jul 25, 2024 •

edited

Loading

aulorbe left a comment •

edited

Loading