Add `codegen/build-clients.sh` script, update `codegen/apis` submodule #49

austin-denoble · 2024-07-22T20:03:17Z

Problem

We're getting ready to release v1.0.0 of the Go SDK. Before we do that, we need to make sure we're generating our control and data plane client code from the proper specifications. When the go-pinecone repo was originally set up, the apis submodule was created and points to a private, Go-specific cut of the OpenAPI and proto files from about 4 months ago.

Since then we're made changes to how we approach client generation, and where our upstream spec files live.

Solution

Add new codegen/ folder to hold both the apis submodule and the build-clients.sh script.
Update .gitmodules to move apis -> codegen/apis.
Add a build-clients.sh script. Originally, just gen called a few shell commands for generating from OpenAPI and protos. The new script handles generation in a fashion similar to our other repos, and allows specifying a specific API version.
Update client.go / client_test.go to handle some changes to underlying types.

My approach was based on the work @jhamon has done in other repos to align our code generation pipelines.

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

There weren't changes needed to the actual SDK interface. Make sure CI runs and our unit & integration tests are passing as expected.

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

…d-clients.sh script for handling building control and data plane clients using a specific version of the API spec, update client.go to match generated internals

austin-denoble · 2024-07-22T20:26:21Z

.gitmodules

+[submodule "codegen/apis"]
+	path = codegen/apis
+	url = [email protected]:pinecone-io/apis.git


Moving to new submodule location, and swapping the underlying repo.

austin-denoble · 2024-07-22T20:29:16Z

codegen/build-clients.sh

@@ -0,0 +1,70 @@
+#!/bin/bash


I was modeling this after the work @jhamon had done in other repos, but this diverges a bit in that I'm not using a docker image, and we're doing both OAS and proto files in here.

Let me know if this doesn't make sense - or any other feedback. I didn't want to make too many changes to the dependencies we were using to generate code.

austin-denoble · 2024-07-22T20:30:03Z

codegen/build-clients.sh

+    echo "Verifying spec version has been provided: $version"
+    if [ -z "$version" ]; then
+        echo "Version is required"
+        exit 1
+    fi 


I should maybe do some actual checking against the folder names under /apis/_build/ maybe. 🤔

austin-denoble · 2024-07-22T20:30:35Z

codegen/build-clients.sh

+
+    oapi-codegen --package=control \
+    --generate types,client \
+    "${oas_file}" > "${control_destination}/control_plane.oas.go"


There's a version in the proto / data plane output, maybe should have that here too.

austin-denoble · 2024-07-22T20:31:01Z

go.mod

@@ -3,7 +3,6 @@ module github.com/pinecone-io/go-pinecone
 go 1.21

 require (
-	github.com/golang/protobuf v1.5.4


I ran go mod tidy locally and this was removed... I think the google.golang.org/protobuf v1.34.1 below covers it?

Yeah, we should prefer google.golang.org according to https://protobuf.dev/reference/go/faq/.

austin-denoble · 2024-07-22T20:31:34Z

justfile

    go install golang.org/x/tools/cmd/godoc@latest
 gen:
-  protoc --experimental_allow_proto3_optional --proto_path=apis/proto --go_opt=module="github.com/pinecone-io/go-pinecone" --go-grpc_opt=module="github.com/pinecone-io/go-pinecone" --go_out=. --go-grpc_out=. apis/proto/pinecone/data/v1/vector_service.proto
-  oapi-codegen --package=control --generate types,client apis/openapi/control/v1/control_v1.yaml > internal/gen/control/control_plane.oas.go
+  ./codegen/build-clients.sh "2024-07"


Calling the script with version from here, we could set a global "API_VERSION" constant somewhere and use that here and elsewhere maybe?

just can also have variables: https://just.systems/man/en/chapter_29.html

austin-denoble · 2024-07-22T20:31:58Z

pinecone/client.go

-			Shards           *control.PodSpecShards   `json:"shards,omitempty"`
-			SourceCollection *string                  `json:"source_collection,omitempty"`
-		}{
+	req.Spec = control.IndexSpec{


Generated anonymous type cleaned up a bit, which is nice.

This is nice. The previous impl was unsightly.

austin-denoble · 2024-07-22T20:36:29Z

justfile

@@ -19,11 +19,10 @@ test-unit:
 bootstrap:
    go install google.golang.org/protobuf/cmd/[email protected]
    go install google.golang.org/grpc/cmd/[email protected]
-    go install github.com/oapi-codegen/oapi-codegen/v2/cmd/[email protected]
+    go install github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@latest
    go install golang.org/x/tools/cmd/godoc@latest
 gen:


just gen now requires just bootstrap so the script has the right dependencies to call... should it call bootstrap itself or is unexpectedly installing dependencies weird?

yeah, that would be weird. bootstrap shouldn't be a requirement for just gen

jhamon

Looks reasonable to me, assuming tests are passing with the newly built output. Is the version being set in X-Pinecone-Api-Version header for control plane actions? For some other languages, I had to augment the generated code by writing the version number somewhere (see python and ts) and then setting up that custom header. The generated java included the version number, but still required some effort to hook it up into headers properly. So it's worth double checking that is being done correctly after this refactoring.

austin-denoble · 2024-07-22T20:54:19Z

Is the version being set in X-Pinecone-Api-Version header for control plane actions? For some other languages, I had to augment the generated code by writing the version number somewhere (see python and ts) and then setting up that custom header.

Good question, I was going to implement the header in a follow up PR, but given what you mentioned I should probably just do it here. Thanks for the heads up, I had wrongly assumed the version number was written in as part of the spec build process and wasn't considering it.

haruska

Minor comments. Looks good!

haruska · 2024-07-22T20:49:03Z

.gitignore

@@ -120,3 +120,4 @@ modules.xml

 go.work*
 .env
+.DS_Store


Should probably just add this to your dev machine as a global gitignore (similar to .idea, .swp files, etc)

haruska · 2024-07-22T20:52:17Z

go.mod

@@ -3,7 +3,6 @@ module github.com/pinecone-io/go-pinecone
 go 1.21

 require (
-	github.com/golang/protobuf v1.5.4


Yeah, we should prefer google.golang.org according to https://protobuf.dev/reference/go/faq/.

haruska · 2024-07-22T20:54:19Z

justfile

@@ -19,11 +19,10 @@ test-unit:
 bootstrap:
    go install google.golang.org/protobuf/cmd/[email protected]
    go install google.golang.org/grpc/cmd/[email protected]
-    go install github.com/oapi-codegen/oapi-codegen/v2/cmd/[email protected]
+    go install github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@latest
    go install golang.org/x/tools/cmd/godoc@latest
 gen:


yeah, that would be weird. bootstrap shouldn't be a requirement for just gen

haruska · 2024-07-22T20:55:21Z

justfile

@@ -19,11 +19,10 @@ test-unit:
 bootstrap:
    go install google.golang.org/protobuf/cmd/[email protected]
    go install google.golang.org/grpc/cmd/[email protected]
-    go install github.com/oapi-codegen/oapi-codegen/v2/cmd/[email protected]


We pinned this to a version to avoid unnecessary codegen updates. It's fine to update but maybe a specific version instead of latest.

Thanks for the catch, I mean to pin to v2.3.0 which is the version they swapped to the new github.com location.

haruska · 2024-07-22T20:56:22Z

justfile

    go install golang.org/x/tools/cmd/godoc@latest
 gen:
-  protoc --experimental_allow_proto3_optional --proto_path=apis/proto --go_opt=module="github.com/pinecone-io/go-pinecone" --go-grpc_opt=module="github.com/pinecone-io/go-pinecone" --go_out=. --go-grpc_out=. apis/proto/pinecone/data/v1/vector_service.proto
-  oapi-codegen --package=control --generate types,client apis/openapi/control/v1/control_v1.yaml > internal/gen/control/control_plane.oas.go
+  ./codegen/build-clients.sh "2024-07"


just can also have variables: https://just.systems/man/en/chapter_29.html

haruska · 2024-07-22T20:57:08Z

pinecone/client.go

 	x := minOne(req.Replicas)
-	return &x
+	return x


is assignment and explicit return necessary?

Nope, lemme clean this up - I ran through here too quick fixing the initial build.

haruska · 2024-07-22T20:58:05Z

pinecone/client.go

-			Shards           *control.PodSpecShards   `json:"shards,omitempty"`
-			SourceCollection *string                  `json:"source_collection,omitempty"`
-		}{
+	req.Spec = control.IndexSpec{


This is nice. The previous impl was unsightly.

…in oapi-codegen to v2.3.0

…enting the API version as a constant, use that as a header for both control and data plane operations

…eaders on the client

austin-denoble · 2024-07-23T15:43:36Z

The generated java included the version number, but still required some effort to hook it up into headers properly. So it's worth double checking that is being done correctly after this refactoring.

Added a generate_version_file() function to the build-clients.sh script. This outputs whichever API version was used to generate the code into a Go file as a constant, which is then imported and used for sending the X-Pinecone-Api-Version with requests.

austin-denoble added 2 commits July 22, 2024 15:13

add codegen folder, move apis submodule to codegen/apis, add new buil…

dd3cf3b

…d-clients.sh script for handling building control and data plane clients using a specific version of the API spec, update client.go to match generated internals

fix configureIndex to align with latest spec

74cac05

austin-denoble commented Jul 22, 2024

View reviewed changes

remove unnecessary echo

b927ace

austin-denoble commented Jul 22, 2024

View reviewed changes

austin-denoble requested review from haruska, aulorbe and jhamon July 22, 2024 20:32

austin-denoble marked this pull request as ready for review July 22, 2024 20:32

austin-denoble commented Jul 22, 2024

View reviewed changes

jhamon approved these changes Jul 22, 2024

View reviewed changes

haruska approved these changes Jul 22, 2024

View reviewed changes

austin-denoble added 5 commits July 22, 2024 18:08

review feedback - update .gitignore, tweak helper function returns, p…

a5ad3e8

…in oapi-codegen to v2.3.0

allow build-clients script to create a new api_version.go file repres…

8179289

…enting the API version as a constant, use that as a header for both control and data plane operations

update env var from TEST_POD_ to TEST_PODS_

67323b2

update unit tests to handle api version header

9d75cc0

remove explicit check for api version header as it is not stored in h…

727580a

…eaders on the client

austin-denoble merged commit be9fe92 into main Jul 23, 2024
3 checks passed

austin-denoble deleted the adenoble/update-api-submodule branch July 23, 2024 15:44

austin-denoble mentioned this pull request Jul 24, 2024

Restructure repository pinecone-io/pinecone-rust-client#35

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `codegen/build-clients.sh` script, update `codegen/apis` submodule #49

Add `codegen/build-clients.sh` script, update `codegen/apis` submodule #49

austin-denoble commented Jul 22, 2024 •

edited

Loading

austin-denoble Jul 22, 2024

austin-denoble Jul 22, 2024

austin-denoble Jul 22, 2024

austin-denoble Jul 22, 2024

austin-denoble Jul 22, 2024

haruska Jul 22, 2024

austin-denoble Jul 22, 2024

haruska Jul 22, 2024

austin-denoble Jul 22, 2024

haruska Jul 22, 2024

austin-denoble Jul 22, 2024

haruska Jul 22, 2024

jhamon left a comment •

edited

Loading

austin-denoble commented Jul 22, 2024

haruska left a comment

haruska Jul 22, 2024

haruska Jul 22, 2024

haruska Jul 22, 2024

haruska Jul 22, 2024

austin-denoble Jul 22, 2024

haruska Jul 22, 2024

haruska Jul 22, 2024

austin-denoble Jul 22, 2024

haruska Jul 22, 2024

austin-denoble commented Jul 23, 2024

Add codegen/build-clients.sh script, update codegen/apis submodule #49

Add codegen/build-clients.sh script, update codegen/apis submodule #49

Conversation

austin-denoble commented Jul 22, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

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

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

jhamon left a comment • edited Loading

Choose a reason for hiding this comment

austin-denoble commented Jul 22, 2024

haruska left a 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

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

austin-denoble commented Jul 23, 2024

Add `codegen/build-clients.sh` script, update `codegen/apis` submodule #49

Add `codegen/build-clients.sh` script, update `codegen/apis` submodule #49

austin-denoble commented Jul 22, 2024 •

edited

Loading

jhamon left a comment •

edited

Loading