feat: getting ready for initial Go SDK #3
Conversation
Because directory names matter, and sdk sounds cooler.
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the CI workflow, Makefile, and Protocol Buffers definitions. Key updates include the addition of a step to install the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
Makefile (3)
8-8: LGTM: Default target renamed appropriately.The default target has been renamed from
alltobuild, which is more descriptive of the action being performed. This change is consistent with the updated.PHONYdeclaration and maintains the same functionality.Consider adding a comment above this line to explicitly state that this is the default target, for improved readability. For example:
# Default target build: go
13-14: LGTM: SDK generation process updated and improved.The changes to the
gotarget enhance the build process by adding SDK generation capabilities. The new echo statement improves user feedback, and the updated protoc command reflects the project's new structure and tooling.Consider the following improvements:
Add error checking to ensure the required tools are installed:
@command -v protoc >/dev/null 2>&1 || { echo >&2 "protoc is required but not installed. Aborting."; exit 1; } @command -v ${GOPATH}/bin/protoc-gen-go >/dev/null 2>&1 || { echo >&2 "protoc-gen-go is required but not installed. Aborting."; exit 1; } @command -v ${GOPATH}/bin/protoc-gen-go-vtproto >/dev/null 2>&1 || { echo >&2 "protoc-gen-go-vtproto is required but not installed. Aborting."; exit 1; }Consider using variables for repeated paths and options to improve maintainability:
PROTOC_GEN_GO := ${GOPATH}/bin/protoc-gen-go PROTOC_GEN_GO_VTPROTO := ${GOPATH}/bin/protoc-gen-go-vtproto PROTOC_OPTS := -I=./sdk --go_out=$(GO_OUT) --go-vtproto_out=$(GO_OUT) PROTOC_PLUGINS := --plugin protoc-gen-go="$(PROTOC_GEN_GO)" --plugin protoc-gen-go-vtproto="$(PROTOC_GEN_GO_VTPROTO)" protoc $(PROTOC_OPTS) $(PROTOC_PLUGINS) ./sdk/*.protoAdd a check to ensure the SDK directory exists:
@[ -d "./sdk" ] || { echo >&2 "SDK directory not found. Aborting."; exit 1; }These changes will make the Makefile more robust and easier to maintain.
Issues Identified in Makefile Changes
The verification process revealed the following issues that need to be addressed to ensure the Makefile changes function correctly:
Missing Go Tools: The required tools
protoc,protoc-gen-go, andprotoc-gen-go-vtprotoare not found inPATHorGOPATH/bin. These are essential for generating Go code from.protofiles.Residual 'proto' References: Found references to the
protodirectory outside of the Makefile (e.g., in.github/workflows/ci.ymlandREADME.md). These may need to be updated to reflect the newsdkdirectory structure.Please address these issues to ensure the build process operates as expected.
🔗 Analysis chain
Line range hint
1-20: Overall assessment: Changes align well with PR objectives.The modifications to the Makefile effectively support the generation of the Go SDK, which is the primary objective of this PR. The changes maintain good Makefile practices and improve the build process by focusing on SDK generation.
To ensure the changes are consistent across the project, please run the following verification script:
This script will help ensure that the changes in the Makefile are consistent with the project structure and that all necessary tools are available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of SDK-related changes across the project. # Test 1: Check if the SDK directory exists if [ ! -d "./sdk" ]; then echo "Error: SDK directory not found. The Makefile assumes its existence." exit 1 fi # Test 2: Verify the presence of .proto files in the SDK directory proto_files=$(find ./sdk -name "*.proto") if [ -z "$proto_files" ]; then echo "Warning: No .proto files found in the SDK directory. The Makefile may not function as expected." fi # Test 3: Check for any remaining references to the old 'proto' directory old_proto_refs=$(grep -R "proto" --exclude=Makefile --exclude-dir=.git .) if [ ! -z "$old_proto_refs" ]; then echo "Warning: Found references to 'proto' directory outside of Makefile. These may need to be updated:" echo "$old_proto_refs" fi # Test 4: Verify the presence of required Go tools required_tools=("protoc" "protoc-gen-go" "protoc-gen-go-vtproto") for tool in "${required_tools[@]}"; do if ! command -v "$tool" &> /dev/null; then echo "Error: Required tool '$tool' not found in PATH or GOPATH/bin." fi done echo "Verification complete."Length of output: 9325
.github/workflows/ci.yml (1)
36-37: Approve the addition ofprotoc-gen-go-vtprotowith a version pinning suggestion.The addition of the
protoc-gen-go-vtprotoinstallation step is appropriate for integratingvtprotobufinto the build process. This aligns with the PR objective of preparing for the initial Go SDK.Consider pinning the version of
protoc-gen-go-vtprototo ensure reproducible builds. For example:- name: Install protoc-gen-go-vtproto run: go install github.com/planetscale/vtprotobuf/cmd/[email protected]Replace
v0.4.0with the desired version.sdk/metrics.proto (1)
18-21: Consider using an enum for theactionfield.The addition of the
actionfield toMetricsGaugeis a good improvement, allowing for more flexible gauge operations. However, using a string for this purpose might not be the most type-safe approach.Consider using an enum instead of a string for better type safety and to prevent invalid values. Here's a suggested implementation:
enum GaugeAction { GAUGE_ACTION_UNSPECIFIED = 0; GAUGE_ACTION_INCREMENT = 1; GAUGE_ACTION_DECREMENT = 2; } message MetricsGauge { string name = 1; GaugeAction action = 2; }This approach would provide compile-time checks and prevent invalid action values.
sdk/http.proto (1)
23-26: LGTM with suggestion: Added insecure field to HTTPClientThe addition of the 'insecure' boolean field is valuable for scenarios involving self-signed certificates. The field is well-placed and properly commented, including a caution about its use.
However, given the potential security implications of disabling TLS host verification, consider adding more comprehensive documentation or safeguards:
- Add a more detailed comment explaining the specific use cases for this option.
- Consider implementing a logging mechanism that warns when this option is used in production environments.
- If possible, add a configuration option to entirely disable this feature in security-sensitive deployments.
Would you like assistance in drafting extended documentation for this field?
sdk/kvstore.proto (1)
79-103: LGTM. Consider adding a default value forreturn_proto.The new
KVStoreKeysmessage and the updatedKVStoreKeysResponsemessage are well-structured and documented. They provide a clear mechanism for retrieving keys from the KV store.Consider adding a default value for the
return_protofield in theKVStoreKeysmessage. This can help clarify the default behavior when the field is not explicitly set. For example:- bool return_proto = 1; + bool return_proto = 1 [default = false];This change would make it clear that by default, the response will be returned as a JSON string unless explicitly set to true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- .github/workflows/ci.yml (1 hunks)
- Makefile (1 hunks)
- proto/sql.proto (0 hunks)
- sdk/http.proto (2 hunks)
- sdk/kvstore.proto (4 hunks)
- sdk/metrics.proto (3 hunks)
- sdk/sql.proto (1 hunks)
- sdk/tarmac.proto (2 hunks)
💤 Files with no reviewable changes (1)
- proto/sql.proto
✅ Files skipped from review due to trivial changes (1)
- sdk/tarmac.proto
🧰 Additional context used
🔇 Additional comments (13)
Makefile (1)
6-6: LGTM: Phony target declaration updated correctly.The
.PHONYdeclaration has been appropriately updated to reflect the change fromalltobuildas the default target. This maintains good Makefile practices by correctly declaring all phony targets..github/workflows/ci.yml (1)
40-40: Verify Makefile changes for thebuildtarget.The change from
make all cleantomake build cleanappears to align with the PR objectives. However, it's important to ensure that thebuildtarget in the Makefile correctly includes all necessary steps for generating language files, especially considering the newvtprotobufintegration.Please run the following script to verify the Makefile changes:
sdk/metrics.proto (2)
29-31: LGTM! Good addition toMetricsHistogram.The addition of the
valuefield toMetricsHistogramis a necessary and well-implemented change. Using a double type is appropriate for histogram values, and the comment clearly explains the purpose of the field.
4-4: LGTM! Verify Go import statements.The
go_packageoption update looks good. It provides a more specific and structured path for the generated Go code.Please ensure that all Go code importing these protobuf definitions is updated to use the new package path. Run the following script to check for any outdated imports:
sdk/http.proto (6)
13-15: LGTM: Added headers field to HTTPClientThe addition of the 'headers' field as a map of string key-value pairs is a good improvement. It allows for flexible specification of custom HTTP headers in the client request.
16-18: LGTM: Added url field to HTTPClientThe addition of the 'url' field as a string is essential for specifying the target URL for the HTTP request. The field is well-placed and properly commented.
19-22: LGTM: Added body field to HTTPClientThe addition of the 'body' field as bytes is crucial for including payload data in the HTTP request. The field is well-defined and properly commented, explaining its purpose clearly.
35-44: LGTM: Comprehensive updates to HTTPClientResponseThe additions to the HTTPClientResponse message (code, headers, and body fields) provide a well-structured representation of HTTP responses. These changes are symmetrical to the additions in the HTTPClient message, which is excellent for consistency and ease of use.
Each field is appropriately numbered and well-commented, clearly explaining its purpose:
- The 'code' field for the HTTP status code
- The 'headers' field for response headers
- The 'body' field for the response payload
This comprehensive structure will greatly facilitate handling of HTTP responses in the SDK.
Line range hint
1-44: Overall assessment: Well-structured and comprehensive updatesThe changes to
sdk/http.protosignificantly enhance the HTTP-related message structures, providing a more comprehensive representation of both HTTP requests and responses. The additions to bothHTTPClientandHTTPClientResponsemessages are symmetrical, logical, and well-documented.Key improvements:
- Updated go_package option for better organization
- Added fields for headers, URL, body, and insecure option in HTTPClient
- Added fields for status code, headers, and body in HTTPClientResponse
These changes align well with the PR objective of preparing for the initial Go SDK, offering a robust foundation for HTTP operations. The new structure should facilitate easier and more flexible HTTP handling in the SDK.
One minor suggestion to consider is enhancing the documentation around the 'insecure' option to ensure its judicious use.
Overall, these changes represent a significant step forward in the SDK's HTTP capabilities.
6-6: LGTM: Updated go_package optionThe go_package option has been appropriately updated to reflect the new organization for the generated Go code. This change aligns with the PR objective of preparing for the initial Go SDK.
To ensure consistency, please verify that any existing Go code importing the old package has been updated. Run the following script to check for any remaining references to the old package:
✅ Verification successful
go_package option change verification
No Go files were found in the repository, ensuring that there are no existing references to the old package path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to the old package path in Go files rg --type go "github.com/tarmac-project/tarmac/proto"Length of output: 202
Script:
#!/bin/bash # Description: List all Go files and search for references to the old package path without using --type go # List all .go files echo "Listing all Go files:" fd --extension go # Search for references without type filtering echo "Searching for 'github.com/tarmac-project/tarmac/proto' in Go files:" rg "github.com/tarmac-project/tarmac/proto" --type goLength of output: 426
sdk/kvstore.proto (3)
27-27: LGTM. Improved readability.The addition of a blank line after the
statusfield in theKVStoreGetResponsemessage improves readability and is consistent with good protobuf formatting practices.
41-41: LGTM. Consistent formatting.The addition of a blank line after the
keyfield in theKVStoreSetmessage improves readability and maintains consistency with the formatting in other messages.
6-6: LGTM. Verify import statements in existing Go code.The
go_packageoption update looks good. It appears to be moving the generated Go code to a more specific package, which is a good practice for organization.Please ensure that all import statements in existing Go code that use these protobuf definitions are updated accordingly. Run the following script to check for any outdated imports:
✅ Verification successful
| // Data is a JSON encoded byte slice of the data returned by the query. | ||
| bytes data = 5; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define Structured Messages for Query Results Instead of JSON-Encoded bytes
The data field in SQLQueryResponse contains JSON-encoded data as a byte slice. Leveraging Protobuf's capability to define structured messages for the query results can enhance type safety, performance, and interoperability. Consider defining a repeated message type to represent each row in the result set.
Because every workflow deserves a good shell companion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci.yml (1)
38-39: Consider pinning the version of protoc-gen-go-vtproto.Adding the installation of protoc-gen-go-vtproto is good as it ensures the necessary tools are available. However, using
@latestmight lead to unexpected behavior if there are breaking changes in future versions.Consider pinning to a specific version for better reproducibility:
- run: go install github.com/planetscale/vtprotobuf/cmd/protoc-gen-go-vtproto@latest + run: go install github.com/planetscale/vtprotobuf/cmd/[email protected]Replace
v0.4.0with the current latest version or the version that's compatible with your project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/ci.yml (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
9-11: LGTM: Good practice to set default shell.Setting the default shell to bash ensures consistency across all steps in the workflow. This is a good practice that can help prevent potential issues caused by different shell behaviors.
43-44: Reconsider the necessity of setting GOPATH.While the GOPATH has been set to a more appropriate location (
/home/runner/go) compared to the previous suggestion (/root/go), it's worth reconsidering whether setting GOPATH is necessary at all.Modern Go projects using modules typically don't require GOPATH to be set explicitly. If GOPATH is indeed necessary for your build process, please clarify why. Otherwise, consider removing this environment variable setting.
You can run the following script to check if GOPATH is used in the build process:
#!/bin/bash # Description: Check if GOPATH is used in the build process # Test: Search for GOPATH usage in Makefile and Go files echo "Searching for GOPATH usage in Makefile:" grep -n GOPATH Makefile echo "Searching for GOPATH usage in .go files:" grep -r -n GOPATH --include="*.go" .If these searches return no results, it might indicate that GOPATH is not necessary for your build process.
42-42: Verify Makefile changes corresponding to this update.The make command has been changed from
all cleantobuild clean. This likely corresponds to changes in the Makefile.Please ensure that:
- The
buildtarget in the Makefile includes all necessary steps previously covered by thealltarget.- No crucial steps are omitted in this transition.
You can run the following script to check the Makefile changes:
✅ Verification successful
[/run_scripts]
Makefile update verified.
The
buildtarget correctly replaces thealltarget and includes all necessary steps as previously defined.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Makefile changes related to 'all' and 'build' targets # Test: Display relevant Makefile changes echo "Makefile changes related to 'all' and 'build' targets:" git diff origin/main -- Makefile | grep -E '(all|build):'Length of output: 210
Script:
#!/bin/bash # Description: Display the full definition of the 'build' target in the Makefile echo "Full definition of the 'build' target:" rg -A5 '^build:' MakefileLength of output: 213
This PR represents a working protobuf collection for the SDK.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes