feat: migrating to protobuf for callbacks #137
Conversation
Tweaking inline variable declaration because, unlike Iron Man, we might need some suits to keep things tidy.
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing support for Protocol Buffers (protobuf) in various packages. Key changes include transitioning from Changes
Possibly related PRs
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 (
|
Decided to join the sequel bandwagon by upgrading our makefile from `docker-compose` to `docker compose`. Much like how Terminator 2 improved upon its predecessor, this should streamline our test setups with more consistent results — hopefully without any Schwarzenegger mishaps.
Tidying up because even test data deserves to look its best.
- Introduces `KVStoreKeys` with marshaling for proto. - Adds missing `--rm` to Docker operations to clean up afterward, because who wants leftover containers? - Includes Makefile tweaks for a tidier build process.
Modified the conditional check to accurately handle empty inputs. Because, sometimes, zero really does mean everything – especially to a bug.
Renamed `he` to `hc` because even receivers deserve a little consistency in life, and we all know it's nice to be called by the right name.
Fine-tuning error messages for a sprinkle of precision. After all, it's better to know a number rather than guess at a mystery wrapped in a test failure.
Refactored test functions to ignore redundant params. Now, who needs names anyway when you're just a "_"?
Refactored the test function to ignore the key param. Who needs names when you're just getting deleted anyway?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🧹 Outside diff range and nitpick comments (14)
pkg/callbacks/httpclient/httpclient.go (2)
31-34: Consider renaming import aliases for clarityThe alias
protois used forgit.luolix.top/tarmac-project/protobuf-go/sdk/http, whilepbis used forgoogle.golang.org/protobuf/proto. Sinceprotois commonly used for the standard protobuf package, this could lead to confusion. Consider swapping the aliases or choosing more descriptive names to improve code readability.
Line range hint
128-171: Ensure consistent error handling incallJSONmethodThe
callJSONmethod has similar error handling patterns as theCallmethod. To improve code maintainability and prevent similar issues, consider updating the error handling incallJSONto return early after errors and limit the response body size.pkg/callbacks/metrics/metrics.go (1)
152-153: Enhance validation by specifying allowed actionsIn the
gaugemethod, the action validation could be improved by defining the allowed actions as constants or an enumeration. This enhances readability and makes future modifications easier.Apply this diff to define allowed actions:
+const ( + actionIncrement = "inc" + actionDecrement = "dec" +) func (m *Metrics) gauge(name string, action string) error { // Verify Name Value if !isMetricNameValid.MatchString(name) { return ErrInvalidMetricName } - if action != "inc" && action != "dec" { + if action != actionIncrement && action != actionDecrement { return fmt.Errorf("invalid action") }pkg/callbacks/sql/sql.go (3)
116-117: Include status code in error returned fromExecWhen returning an error from the
Execmethod:return rsp, fmt.Errorf("%s", r.Status.Status)Including the status code in the error message can provide more context to the caller. Consider modifying the error message to include the code:
Apply this diff:
-return rsp, fmt.Errorf("%s", r.Status.Status) +return rsp, fmt.Errorf("%d: %s", r.Status.Code, r.Status.Status)This helps in debugging by clearly indicating the error code along with the message.
65-67: Provide more informative unmarshalling error messagesWhen unmarshalling the input in the
Execmethod:err := pb.Unmarshal(b, msg) if err != nil { return []byte(""), fmt.Errorf("unable to unmarshal database:exec request") }Including the underlying error message can aid in debugging while still maintaining security:
Apply this diff to include the error detail:
-return []byte(""), fmt.Errorf("unable to unmarshal database:exec request") +return []byte(""), fmt.Errorf("unable to unmarshal database:exec request: %v", err)Be cautious to avoid leaking sensitive information in error messages.
247-252: Avoid reallocating slices within the loopIn the
querymethod, the slicescolNamesanddataare allocated inside the loop:for rows.Next() { colNames := make([]interface{}, len(columns)) data := make([]interface{}, len(columns)) for i := range colNames { data[i] = &colNames[i] } // ... }Allocating these slices on each iteration can impact performance. Consider moving the allocation outside the loop to reuse the slices:
Apply this diff:
- for rows.Next() { + colNames := make([]interface{}, len(columns)) + data := make([]interface{}, len(columns)) + for i := range colNames { + data[i] = &colNames[i] + } + for rows.Next() { - colNames := make([]interface{}, len(columns)) - data := make([]interface{}, len(columns)) - for i := range colNames { - data[i] = &colNames[i] - } // ... }This optimization reduces memory allocations and can improve performance.
pkg/callbacks/sql/sql_test.go (2)
14-16: Organize import statements according to Go conventionsThe import statements can be better organized for readability. Go convention suggests grouping standard library imports, third-party imports, and local imports separately.
Consider reorganizing the imports as follows:
import ( "database/sql" "encoding/base64" "fmt" "strconv" "testing" _ "github.com/go-sql-driver/mysql" "github.com/pquerna/ffjson/ffjson" pb "google.golang.org/protobuf/proto" proto "github.com/tarmac-project/protobuf-go/sdk/sql" "github.com/tarmac-project/tarmac" )
165-165: Consistent error messages on unmarshallingThe error messages for unmarshalling responses are inconsistent across the file. For better readability and maintenance, consider making them consistent.
For example, unify the error messages to:
- t.Fatalf("Error parsing returned query response") + t.Fatalf("Error unmarshalling query response")Apply similar changes to lines 88, 141, 176, 216, 303, 324, and 373.
Also applies to: 88-88, 141-141, 176-176, 216-216, 324-324, 303-303, 373-373
pkg/callbacks/metrics/metrics_test.go (3)
Line range hint
267-432: Ensure consistency between 'Key' and 'name' in JSON payloadsIn some test cases, the
Keyfield and thenamewithin the JSON payload do not match. This inconsistency might lead to confusion or incorrect test validations.For example:
- Line 411:
Key:"json_happy_path"JSON:{"name":"json_happy_path_histo","Value":0.11231}- Line 438:
Key:"json_happy_path"JSON:{"name":"json_happy_path_histo"}Recommendation:
Ensure that the
Keyand thenamein the JSON payload are consistent across all test cases.Apply the following diff to correct the
Keyfields:- Key: "json_happy_path", + Key: "json_happy_path_histo",
Line range hint
267-432: Consider refactoring to reduce code duplication in test casesThe test functions
TestMetricsCallbackandTestMetricsCallbackJSONhave similar structures and share common logic. Refactoring the tests to extract common functionalities into helper functions would improve maintainability and reduce duplication.Recommendation:
- Create helper functions for:
- Initializing test cases
- Executing callbacks based on the
Callbacktype- Verifying metric existence
- Parameterize differences such as input formats (protobuf vs. JSON) to reuse as much code as possible.
This refactoring will make the tests cleaner and easier to extend in the future.
6-7: Ensure proper grouping and ordering of importsTo adhere to Go's import conventions, separate standard library imports from third-party imports with a blank line. Also, ensure that imports are ordered alphabetically within their groups.
Consider rearranging the imports as follows:
package metrics import ( + "testing" proto "github.com/tarmac-project/protobuf-go/sdk/metrics" pb "google.golang.org/protobuf/proto" - "testing" )pkg/callbacks/kvstore/kvstore_test.go (3)
386-466: Provide unique and descriptive test case namesMultiple test cases in
TestKVStoreJSONshare the samenamefield, such as "Happy Path" or "Invalid Request JSON". This can make it difficult to identify which specific test case failed during execution.Consider giving each test case a unique and descriptive name to improve test clarity. For example:
- "Happy Path - Get"
- "Happy Path - Set"
- "Invalid JSON - Get Request"
Line range hint
350-382: Use dynamic mock configurations for JSON testsThe mock
kvinTestKVStoreJSONis configured with hardcoded responses based on specific keys. This limits test flexibility and may not cover all test scenarios.Refactor to use dynamic mock configurations per test case, similar to
TestKVStore. This allows each test to define its own mock behavior, improving test coverage and maintainability.
Line range hint
470-536: Capture loop variablecwhen running JSON subtestsIn the
TestKVStoreJSONfunction, the loop variablecis captured by the closures passed tot.Run. This can cause all subtests to reference the same test case data.Capture
cwithin the loop to ensure each subtest operates on the correct data:for _, c := range kc { + c := c switch c.call { case "Set": t.Run(c.name+" Set", func(t *testing.T) { // Test code }) case "Get": t.Run(c.name+" Get", func(t *testing.T) { // Test code }) // Additional cases } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
- Makefile (1 hunks)
- go.mod (3 hunks)
- pkg/callbacks/httpclient/httpclient.go (2 hunks)
- pkg/callbacks/httpclient/httpclient_test.go (12 hunks)
- pkg/callbacks/kvstore/kvstore.go (6 hunks)
- pkg/callbacks/kvstore/kvstore_test.go (2 hunks)
- pkg/callbacks/metrics/metrics.go (2 hunks)
- pkg/callbacks/metrics/metrics_test.go (8 hunks)
- pkg/callbacks/sql/sql.go (4 hunks)
- pkg/callbacks/sql/sql_test.go (2 hunks)
- testdata/function/Makefile (1 hunks)
- testdata/kv/Makefile (1 hunks)
- testdata/logger/Makefile (1 hunks)
- testdata/sql/Makefile (1 hunks)
✅ Files skipped from review due to trivial changes (5)
- Makefile
- testdata/function/Makefile
- testdata/kv/Makefile
- testdata/logger/Makefile
- testdata/sql/Makefile
🧰 Additional context used
🔇 Additional comments (11)
go.mod (5)
3-3: LGTM: Go version updateUpdating the Go version from 1.22.3 to 1.22.6 is a good practice. This minor version update likely includes bug fixes and performance improvements, which can benefit the project.
Line range hint
1-124: Overall, the changes in go.mod align well with the PR objectiveThe updates to the Go version and dependencies in this file are consistent with the goal of migrating to protobuf for callbacks. The addition of protobuf-related dependencies and the update to the latest protobuf library version are appropriate steps for this migration.
Key points:
- Go version updated to 1.22.6
- Added github.com/tarmac-project/protobuf-go dependency
- Updated google.golang.org/protobuf to v1.34.2
- Added indirect dependency on github.com/planetscale/vtprotobuf
These changes set a good foundation for the protobuf migration. Just ensure to follow up on the suggested verifications to maintain code compatibility and understand the impact of new dependencies.
25-25: Protobuf library update looks good, verify compatibilityUpdating
google.golang.org/protobuffrom v1.33.0 to v1.34.2 is a good step in line with the PR objective. This update likely includes improvements and bug fixes related to protobuf handling.However, it's important to ensure that this update doesn't introduce any breaking changes that could affect the existing codebase.
To verify the compatibility of this update with the existing codebase, you can run:
#!/bin/bash # Search for any uses of deprecated or removed protobuf APIs rg --type go 'proto\.' | grep -v 'v1.34'If this search returns results, review them to ensure they're still compatible with the new protobuf version.
80-80: Investigate the source of the new indirect dependencyThe addition of
github.com/planetscale/vtprotobuf v0.6.0as an indirect dependency is noteworthy. vtprotobuf is a protobuf compiler that generates faster marshaling and unmarshaling code, which aligns with the PR objective and may provide performance benefits.However, it's important to understand which direct dependency is pulling in this indirect dependency.
To investigate which direct dependency is requiring vtprotobuf, you can run:
#!/bin/bash # Find direct dependencies that might be using vtprotobuf rg --type go 'github.com/planetscale/vtprotobuf'This will help identify which part of the codebase or which direct dependency is utilizing vtprotobuf, allowing for a better understanding of its impact on the project.
21-21: Addition of protobuf-go dependency looks good, but consider using a released versionThe addition of
github.com/tarmac-project/protobuf-goaligns with the PR objective of migrating to protobuf for callbacks. However, the current version string (v0.0.0-20241006222441-51664fb11f56) suggests it's pointing to a specific commit rather than a released version.While this is acceptable, consider using a released version if available for better stability and easier dependency management.
To check if there are any released versions of this package, you can run:
If there are no released versions, this approach is fine, but it would be good to plan for creating releases in the future.
✅ Verification successful
Addition of protobuf-go dependency approved.
The addition of
github.com/tarmac-project/protobuf-gois appropriate for migrating to protobuf for callbacks. Currently, there are no released versions or tags available for this package, which is why the dependency points to a specific commit (v0.0.0-20241006222441-51664fb11f56).While this approach is acceptable for now, it's recommended to plan for creating formal releases in the future to ensure better stability and ease of dependency management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash gh repo view github.com/tarmac-project/protobuf-go --json releases --jq '.releases[].tagName'Length of output: 48
Script:
#!/bin/bash # Fetching all tags from the protobuf-go repository gh api repos/tarmac-project/protobuf-go/tags --paginate | jq '.[].name'Length of output: 73
pkg/callbacks/httpclient/httpclient.go (1)
73-77:⚠️ Potential issueExercise caution with
InsecureSkipVerifySetting
InsecureSkipVerify: truedisables TLS certificate verification, exposing the client to potential man-in-the-middle attacks. Ensure that allowing users to setmsg.Insecureis acceptable within your security context. Consider documenting this behavior clearly and validating the necessity of this option when enabled.pkg/callbacks/sql/sql.go (1)
127-128: Ensure secure handling when falling back to JSON unmarshallingIn the
Querymethod, if Protobuf unmarshalling fails, the code falls back to JSON:// Fallback to JSON if proto fails return db.queryJSON(b)While this maintains backward compatibility, it's important to ensure that this fallback mechanism does not introduce security vulnerabilities, such as allowing malformed inputs or unintended data processing.
Consider reviewing the input validation logic to ensure that only expected and safe inputs are processed.
pkg/callbacks/kvstore/kvstore.go (1)
75-76: Verify the fallback to JSON handling on Protobuf unmarshal errorsIn the
Get,Set, andDeletemethods, if the Protobuf unmarshal fails, the code falls back to the JSON-based handling for backward compatibility. Ensure that this fallback mechanism is still necessary and correctly implemented. If all clients have migrated to Protobuf, you might consider deprecating the JSON fallback.You can run the following script to check how often the JSON fallback is used:
Also applies to: 157-158, 246-247
pkg/callbacks/metrics/metrics_test.go (3)
16-17: Fields 'Action' and 'Value' added to 'MetricsCase' structThe addition of the
ActionandValuefields to theMetricsCasestruct enhances the test cases to handle gauges and histograms effectively. This allows for more granular testing of different metric types.
21-178: Comprehensive test cases added for metrics callbacksThe expanded test cases within
TestMetricsCallbackthoroughly cover various scenarios, including edge cases for counters, gauges, and histograms. This comprehensive approach strengthens the reliability of the metrics callbacks.
258-265: Introduction of 'MetricsCaseJSON' struct for JSON test casesThe new
MetricsCaseJSONstruct simplifies the handling of JSON-based test cases, allowing for clear separation from protobuf-based tests and improving the readability of the test code.
Gave our HTTP client some empathy—now it not only fails, but also politely explains itself.
Just moved some stuff around to keep the testdata more organized. When a single directory just can't handle the fame, split it!
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Upgraded TinyGo version and enhanced SQL handling to improve stability. Logging now adds a touch of empathy, so errors feel warm and cozy.
Refactored Docker volume path in the Makefile to distinguish it from the rest. Because even build directories deserve a touch of individuality!
Tweaked the volume mount path because even directories get lost sometimes. Don't worry, they're safe at "/build" now!
Replaced `proto` methods with `VT` for better performance. Now, it's faster than your morning coffee!
Updated Makefiles to use the latest TinyGo release. I guess we all want to live on the edge now and then.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #137 +/- ##
==========================================
- Coverage 80.16% 79.38% -0.79%
==========================================
Files 14 14
Lines 1467 1775 +308
==========================================
+ Hits 1176 1409 +233
- Misses 229 285 +56
- Partials 62 81 +19 ☔ View full report in Codecov by Sentry. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Replaced fmt.Errorf with errors package—because sometimes even errors need a makeover. Also, JSON keys get a lower-case makeover for their "value" personality.
Straighten out column handling because spaces are rebels without a cause.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (31)
testdata/sdkv1/kv/docker-compose.yml (2)
4-4: Consider specifying an image tag.While using the 'madflojo/tarmac' image is appropriate for this Tarmac example, it's generally a good practice to specify a particular version or tag (e.g., 'madflojo/tarmac:latest' or 'madflojo/tarmac:v1.0.0'). This ensures consistency across different environments and prevents unexpected changes when the image is updated.
11-12: Consider more specific volume mounting.While mounting the current directory (".") to "/functions" in the container allows for easy updating of function code, it might include unnecessary files. Consider mounting only the specific directories or files needed for the functions to run. This can improve security by limiting access and potentially improve performance.
For example, if only a specific subdirectory is needed:
volumes: - "./functions:/functions"testdata/base/kv/Makefile (3)
3-5: LGTM! Consider adding a .PHONY declaration.The
buildtarget is well-structured and uses Docker to ensure a consistent build environment. The command is clear and includes necessary settings for volume mounting and working directory.Consider adding a
.PHONYdeclaration for this target at the beginning of the Makefile:.PHONY: build docker-compose runThis prevents conflicts with any files named
buildand explicitly declares the target as a recipe to be executed rather than a file to be built.
10-10: LGTM! Consider adding a comment for clarity.The
runtarget effectively combines thebuildanddocker-composesteps in the correct order. This ensures that the WebAssembly binary is built before deploying the containers.For improved clarity, consider adding a comment explaining the purpose of this target:
# Build the WebAssembly binary and start the Docker containers run: build docker-composeThis helps other developers understand the purpose and flow of the
runtarget at a glance.
1-10: Overall, well-structured Makefile with room for minor enhancements.This Makefile effectively manages the build and execution process for Go example applications using Tarmac WASM functions. It leverages Docker for both building and running, ensuring consistency across different environments.
Consider the following enhancements to improve the overall quality and maintainability of the Makefile:
- Add a
.PHONYdeclaration at the beginning for all non-file targets.- Include a
cleantarget to remove built artifacts:clean: rm -f tarmac.wasm- Add a
helptarget to display available commands:help: @echo "Available targets:" @echo " build - Build the WebAssembly binary" @echo " up - Start Docker containers" @echo " run - Build and start containers" @echo " clean - Remove built artifacts"- Consider adding a
.DEFAULT_GOAL := helpat the beginning to makehelpthe default target.These additions would make the Makefile more user-friendly and easier to maintain.
testdata/base/logger/Makefile (2)
3-5: LGTM! Consider adding a.PHONYdeclaration.The
buildtarget is well-structured and uses Docker to ensure consistent TinyGo builds. This is a good practice for WebAssembly development.Consider adding a
.PHONYdeclaration for thebuildtarget at the beginning of the Makefile:.PHONY: buildThis prevents conflicts with any files named "build" and clarifies that it's a phony target.
10-10: LGTM! Consider adding a.PHONYdeclaration.The
runtarget is well-structured, combining thebuildanddocker-composetargets in the correct order. This provides a convenient way to build and run the project with a single command.Consider adding a
.PHONYdeclaration for theruntarget at the beginning of the Makefile:.PHONY: runThis prevents conflicts with any files named "run" and clarifies that it's a phony target.
testdata/base/sql/Makefile (3)
3-5: LGTM! Consider version pinning and error handling.The
buildtarget effectively uses Docker to run TinyGo for building the WASM binary. This approach ensures a consistent build environment and is suitable for creating small WASM binaries.Consider the following improvements:
- Use a specific version of TinyGo instead of
latestto ensure consistent builds over time.- Add a check to ensure Docker is installed and running before executing the build command.
Here's an example of how you could implement these suggestions:
build: @if ! command -v docker &> /dev/null; then \ echo "Docker is not installed. Please install Docker and try again."; \ exit 1; \ fi @if ! docker info &> /dev/null; then \ echo "Docker is not running. Please start Docker and try again."; \ exit 1; \ fi docker run --rm -v `pwd`/:/build -w /build -u root tinygo/tinygo:0.28.1 tinygo build -o /build/tarmac.wasm -target wasi /build/main.goThis modification adds error checking for Docker installation and runtime, and pins the TinyGo version to 0.28.1 (adjust as needed).
7-8: LGTM! Consider adding error handling and flags.The
docker-composetarget effectively starts the services using Docker Compose.Consider the following improvements:
- Add version checking for Docker Compose.
- Include useful flags like
--detachfor background execution or--remove-orphansfor cleanup.- Add error handling.
Here's an example of how you could implement these suggestions:
docker-compose: @if ! command -v docker compose &> /dev/null; then \ echo "Docker Compose is not installed. Please install Docker Compose and try again."; \ exit 1; \ fi docker compose version docker compose up --detach --remove-orphansThis modification adds error checking for Docker Compose installation, prints the version for debugging purposes, and includes useful flags for the
upcommand.
10-10: LGTM! Consider adding .PHONY and an option to skip build.The
runtarget effectively combines the build and execution steps into a single command, ensuring that the build is performed before starting the services.Consider the following improvements:
- Add a
.PHONYdirective for non-file targets to avoid conflicts with files of the same name and to improve performance.- Introduce an option to skip the build step if the WASM file already exists and is up-to-date.
Here's an example of how you could implement these suggestions:
.PHONY: build docker-compose run run: @if [ ! -f tarmac.wasm ] || [ main.go -nt tarmac.wasm ]; then \ $(MAKE) build; \ else \ echo "tarmac.wasm is up-to-date, skipping build"; \ fi $(MAKE) docker-composeThis modification adds the
.PHONYdirective and introduces a check to skip the build step iftarmac.wasmexists and is newer thanmain.go.testdata/sdkv1/kv/Makefile (2)
3-5: LGTM! Consider adding a .PHONY declaration.The
buildtarget is well-structured and uses Docker to run TinyGo, ensuring consistency across environments. The command includes necessary flags and targets the appropriate WASI output.Consider adding a
.PHONYdeclaration at the beginning of the Makefile to explicitly declare the phony targets:.PHONY: build docker-compose runThis helps Make understand that these are not file targets, potentially improving performance and preventing conflicts with files of the same name.
10-10: LGTM! Consider adding error handling.The
runtarget effectively sequences thebuildanddocker-composetargets, ensuring that the WASM binary is built before running the containers.For enhanced robustness, consider adding basic error handling. Here's a suggestion:
run: @$(MAKE) build || (echo "Build failed"; exit 1) @$(MAKE) docker-compose || (echo "Docker Compose failed"; exit 1)This change will:
- Use
@to suppress command echoing for cleaner output- Provide specific error messages if either step fails
- Stop execution if any step fails, preventing partial runs
testdata/sdkv1/logger/Makefile (3)
3-5: LGTM with a minor suggestion for improvement.The
buildtarget effectively uses Docker to run the TinyGo build process, ensuring consistency across different environments. The command correctly mounts the current directory and uses appropriate flags for output and target specifications.Consider using a specific version of the TinyGo image instead of
latestto ensure reproducible builds:- docker run --rm -v `pwd`/:/build -w /build -u root tinygo/tinygo:latest tinygo build -o /build/tarmac.wasm -target wasi /build/main.go + docker run --rm -v `pwd`/:/build -w /build -u root tinygo/tinygo:0.28.1 tinygo build -o /build/tarmac.wasm -target wasi /build/main.goReplace
0.28.1with the desired version number.
7-8: Consider renaming the target and adding options for better usability.The target uses the correct modern
docker composesyntax. However, there are a couple of points to consider:
- The target name
docker-compose(with hyphen) doesn't match the commanddocker compose(without hyphen). This might cause confusion.- The command will run in the foreground and block the terminal. Adding the
-dflag would run it in detached mode.Consider applying these changes:
-docker-compose: - docker compose up +docker-compose-up: + docker compose up -dThis change improves consistency between the target name and the command, and allows the containers to run in the background.
10-10: Update target dependencies and consider adding a stop target.The
runtarget correctly ensures that the build is performed before starting the containers. However, there are a couple of improvements we can make:
- If we rename the
docker-composetarget todocker-compose-upas suggested earlier, this target will need to be updated.- It would be helpful to have a way to stop the containers.
Consider applying these changes:
-run: build docker-compose +run: build docker-compose-up + +stop: + docker compose downThese changes ensure consistency with the renamed
docker-compose-uptarget and provide a way to stop the containers when needed.testdata/sdkv1/sql/Makefile (3)
3-5: LGTM with a minor security suggestion.The
buildtarget effectively uses Docker to run TinyGo for compiling the Go source file into a WebAssembly binary. This approach ensures a consistent build environment and is appropriate for WebAssembly compilation.Consider removing the
-u rootflag from the Docker command to avoid running as the root user inside the container, which is generally considered a security best practice. If root permissions are necessary, consider using a non-root user and only elevating privileges where required.
7-8: LGTM with a minor suggestion for flexibility.The
docker-composetarget correctly uses the moderndocker compose upcommand to start the services defined in the docker-compose.yml file.Consider adding an option to run the services in detached mode. You could modify the target to accept an optional argument:
docker-compose: docker compose up $(DOCKER_COMPOSE_FLAGS)This allows users to run
make docker-compose DOCKER_COMPOSE_FLAGS="-d"for background execution when needed, while maintaining the default foreground behavior.
1-10: Well-structured Makefile with clear targets.This Makefile provides an excellent structure for building and running a Go application with Tarmac WASM functions. It effectively leverages Docker and Docker Compose to ensure a consistent environment for both building and running the application. The targets are logically organized and named, making it easy to understand and use.
To further improve the Makefile, consider adding a brief comment for each target explaining its purpose. This would enhance readability and make it easier for new contributors to understand the build process. For example:
# Builds the WebAssembly binary using TinyGo build: ... # Starts the services using Docker Compose docker-compose: ... # Builds the binary and starts the services run: build docker-composetestdata/sdkv1/logger/main.go (2)
10-10: Consider dependency injection as an alternative to global variables.The global variable
tarmacis declared correctly. However, for better testability and modularity, consider using dependency injection instead of a global variable. This would involve passing thesdk.Tarmacinstance as a parameter to functions that need it.Here's an example of how you could refactor this using dependency injection:
type App struct { tarmac *sdk.Tarmac } func NewApp(tarmac *sdk.Tarmac) *App { return &App{tarmac: tarmac} } func (a *App) Handler(payload []byte) ([]byte, error) { // Use a.tarmac instead of the global tarmac // ... } func main() { tarmac, err := sdk.New(sdk.Config{Namespace: "tarmac", Handler: nil}) if err != nil { return } app := NewApp(tarmac) tarmac.SetHandler(app.Handler) // ... }
22-28: LGTM: Handler function is correct, consider adding a clarifying comment.The Handler function is implemented correctly for use with the Tarmac SDK. It properly logs the incoming payload and returns it unchanged, which is suitable for testing purposes.
Consider adding a comment to explain why the payload is returned unchanged:
func Handler(payload []byte) ([]byte, error) { // Log the payload tarmac.Logger.Info(fmt.Sprintf("Testdata function - %s", payload)) // Return the payload unchanged for testing purposes return payload, nil }testdata/sdkv1/kv/main.go (2)
22-28: LGTM: Handler function and KV Set operation are correct.The Handler function is correctly implemented for use with the Tarmac SDK. The KV Set operation and its error handling are appropriate.
Note that the
payloadparameter is currently unused. If this is intentional for the test, consider adding a comment explaining why, or use the blank identifier_to explicitly ignore it:-func Handler(payload []byte) ([]byte, error) { +func Handler(_ []byte) ([]byte, error) {
29-39: Improve error message for data validation.The KV Get operation and basic data validation are implemented correctly. However, the error message when the data length doesn't match could be more informative.
Consider updating the error message to include more details:
if len(data) != len([]byte("i am a little teapot")) { - return []byte(""), fmt.Errorf("not able to fetch data from KVStore") + return []byte(""), fmt.Errorf("data length mismatch: got %d bytes, expected %d bytes", len(data), len([]byte("i am a little teapot"))) }This will provide more context about why the validation failed, making debugging easier.
testdata/tarmac.json (1)
21-21: LGTM: Filepath updated correctly for 'func' function, but note the namingThe filepath for the 'func' function has been updated to include the new 'base' directory, which is consistent with the overall restructuring.
However, it's worth noting that the function name 'func' differs from the directory name 'function' in the filepath. This might be intentional, but it could potentially lead to confusion. Consider aligning the names if there's no specific reason for the difference.
testdata/sdkv1/tarmac.json (1)
25-69: Routes are well-defined and cover various scenarios.The routes section is comprehensive and well-structured, covering initialization, HTTP endpoints, scheduled tasks, and direct function invocation. This variety is excellent for testing purposes.
Consider the following enhancement:
- For consistency and clarity, you might want to add a comment or description field to each route, especially for the scheduled_task and function routes. This could help other developers understand the purpose of each route more quickly.
Example for the scheduled_task route:
{ "type": "scheduled_task", "frequency": 15, "function": "default", "description": "Runs the default function every 15 minutes for periodic maintenance" }pkg/wasm/wasm_test.go (1)
122-122: LGTM: Consistent file path update with a suggestion for improvementThe file path update in the
TestWASMExecutionfunction is consistent with the changes made in theTestWASMModuleCreationfunction, ensuring that both tests use the same updated path "/testdata/base/logger/tarmac.wasm".Consider defining a constant for the WASM file path to improve maintainability. This would make it easier to update the path across all tests if needed in the future. For example:
const testWASMFilePath = "/testdata/base/logger/tarmac.wasm" // Then use it in your tests like this: Filepath: testWASMFilePath,This change would make the tests more resistant to future path changes and reduce the risk of inconsistencies.
pkg/callbacks/httpclient/httpclient.go (1)
57-91: LGTM: Protobuf support with JSON fallbackThe changes to support protobuf while maintaining backward compatibility with JSON are well-implemented. The error handling and HTTP client creation look appropriate.
Consider adding a comment to explain the JSON fallback for clarity:
if err != nil { + // Fallback to JSON parsing for backward compatibility return hc.callJSON(b) }pkg/callbacks/sql/sql.go (1)
228-272: LGTM: New query helper method with a suggestionThe new
queryhelper method is well-implemented, providing a reusable way to execute SQL queries and process results. The use of a map for results allows for flexible column handling.However, consider adding error handling within the
rows.Next()loop to catch any errors that might occur during iteration:for rows.Next() { + if rows.Err() != nil { + return nil, nil, fmt.Errorf("error during row iteration: %v", rows.Err()) + } // ... existing code ... }This addition would help catch any errors that occur during the iteration process, improving the robustness of the method.
pkg/callbacks/kvstore/kvstore.go (2)
71-111: LGTM: Updated Get method with protobuf supportThe
Getmethod has been successfully updated to support both protobuf and JSON inputs, ensuring backward compatibility. The error handling is comprehensive, and the response structure using protobuf messages is appropriate.Consider adding a comment explaining the fallback to JSON for clarity:
err := pb.Unmarshal(b, msg) if err != nil { + // Fallback to JSON for backward compatibility return k.getJSON(b) }
319-348: LGTM with suggestion: Updated Keys method with protobuf supportThe
Keysmethod has been successfully updated to support both protobuf and JSON inputs. The error handling is comprehensive, including appropriate status codes.For consistency with other methods, consider using a similar unmarshal approach:
- if len(b) == 0 { + msg := &proto.KVStoreKeys{} + err := pb.Unmarshal(b, msg) + if err != nil { return k.keysJSON(b) }This change would make the
Keysmethod more consistent with the other methods in terms of input handling.pkg/app/app_test.go (1)
Line range hint
1-507: Summary: Consistent updates to WASM function pathsThe changes in this file consistently update the
wasm_functionpath from/testdata/default/tarmac.wasmto/testdata/base/default/tarmac.wasmacross all test functions. This modification appears to be part of a larger refactoring effort to reorganize the test data directory structure.While the core logic of the tests remains unchanged, it's crucial to ensure that:
- The new path is correct and the WASM file exists at the specified location.
- All tests, including those involving TLS and mTLS configurations, continue to function as expected with the updated path.
Consider running the entire test suite to verify that these path changes haven't inadvertently affected any test outcomes.
testdata/base/sql/main.go (1)
56-56: Typographical correction in the return messageThe return message "Howdie" might be a typo. If you intended to greet with "Howdy," consider correcting it.
Apply this diff to fix the typo:
return []byte("Howdie"), nil +// Should it be "Howdy"? +return []byte("Howdy"), nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
testdata/base/default/go.sumis excluded by!**/*.sumtestdata/base/fail/go.sumis excluded by!**/*.sumtestdata/base/function/go.sumis excluded by!**/*.sumtestdata/base/kv/go.sumis excluded by!**/*.sumtestdata/base/logger/go.sumis excluded by!**/*.sumtestdata/base/sql/go.sumis excluded by!**/*.sumtestdata/base/successafter5/go.sumis excluded by!**/*.sumtestdata/sdkv1/kv/go.sumis excluded by!**/*.sumtestdata/sdkv1/logger/go.sumis excluded by!**/*.sumtestdata/sdkv1/sql/go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
- Makefile (1 hunks)
- pkg/app/app_test.go (6 hunks)
- pkg/app/server_test.go (8 hunks)
- pkg/callbacks/httpclient/httpclient.go (2 hunks)
- pkg/callbacks/kvstore/kvstore.go (6 hunks)
- pkg/callbacks/metrics/metrics_test.go (8 hunks)
- pkg/callbacks/sql/sql.go (4 hunks)
- pkg/callbacks/sql/sql_test.go (2 hunks)
- pkg/wasm/wasm_test.go (5 hunks)
- testdata/base/default/Makefile (1 hunks)
- testdata/base/fail/Makefile (1 hunks)
- testdata/base/function/Makefile (1 hunks)
- testdata/base/kv/Makefile (1 hunks)
- testdata/base/logger/Makefile (1 hunks)
- testdata/base/sql/Makefile (1 hunks)
- testdata/base/sql/go.mod (1 hunks)
- testdata/base/sql/main.go (1 hunks)
- testdata/base/successafter5/Makefile (1 hunks)
- testdata/logger/Makefile (0 hunks)
- testdata/sdkv1/kv/Makefile (1 hunks)
- testdata/sdkv1/kv/docker-compose.yml (1 hunks)
- testdata/sdkv1/kv/go.mod (1 hunks)
- testdata/sdkv1/kv/main.go (1 hunks)
- testdata/sdkv1/logger/Makefile (1 hunks)
- testdata/sdkv1/logger/docker-compose.yml (1 hunks)
- testdata/sdkv1/logger/go.mod (1 hunks)
- testdata/sdkv1/logger/main.go (1 hunks)
- testdata/sdkv1/sql/Makefile (1 hunks)
- testdata/sdkv1/sql/docker-compose.yml (1 hunks)
- testdata/sdkv1/tarmac.json (1 hunks)
- testdata/sql/Makefile (0 hunks)
- testdata/successafter5/Makefile (0 hunks)
- testdata/tarmac-fail.json (1 hunks)
- testdata/tarmac.json (1 hunks)
💤 Files with no reviewable changes (3)
- testdata/logger/Makefile
- testdata/sql/Makefile
- testdata/successafter5/Makefile
✅ Files skipped from review due to trivial changes (6)
- testdata/base/default/Makefile
- testdata/base/sql/go.mod
- testdata/sdkv1/kv/go.mod
- testdata/sdkv1/logger/docker-compose.yml
- testdata/sdkv1/logger/go.mod
- testdata/sdkv1/sql/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
📓 Learnings (3)
pkg/callbacks/kvstore/kvstore.go (2)
Learnt from: madflojo PR: tarmac-project/tarmac#137 File: pkg/callbacks/kvstore/kvstore.go:180-182 Timestamp: 2024-10-13T16:34:42.582Z Learning: In the `kvstore` package (`pkg/callbacks/kvstore/kvstore.go`), it's acceptable to include internal error details in client responses. Internal errors can be included in `rsp.Status.Status` when handling errors in methods like `Set`, `Get`, and `Delete`.Learnt from: madflojo PR: tarmac-project/tarmac#137 File: pkg/callbacks/kvstore/kvstore.go:93-95 Timestamp: 2024-10-13T16:33:49.678Z Learning: In the Tarmac project (Go language), returning internal error details in client responses is acceptable.pkg/callbacks/sql/sql.go (2)
Learnt from: madflojo PR: tarmac-project/tarmac#137 File: pkg/callbacks/sql/sql.go:273-276 Timestamp: 2024-10-13T16:45:32.006Z Learning: In the Tarmac project, specifically in the file `pkg/callbacks/sql/sql.go`, it's acceptable to execute SQL queries directly in the `exec` method without using parameterized queries, as per the project's design and use case.Learnt from: madflojo PR: tarmac-project/tarmac#137 File: pkg/callbacks/sql/sql.go:91-95 Timestamp: 2024-10-13T16:40:10.875Z Learning: In the `Exec` method in `pkg/callbacks/sql/sql.go`, when data is available but `RowsAffected()` or `LastInsertId()` returns an error, it's acceptable to use status code 206 to indicate partial content.pkg/callbacks/sql/sql_test.go (1)
Learnt from: madflojo PR: tarmac-project/tarmac#137 File: pkg/callbacks/sql/sql_test.go:107-107 Timestamp: 2024-10-13T16:47:26.151Z Learning: In `pkg/callbacks/sql/sql_test.go`, within the "Create Table" and "Delete Table" tests, using `db.Query` for non-SELECT statements is acceptable, as various scenarios are being intentionally tested.
🔇 Additional comments (62)
testdata/sdkv1/kv/docker-compose.yml (3)
1-1: LGTM: Appropriate Docker Compose version.The use of Docker Compose file format version 3.8 is appropriate for this configuration. It supports all the features used in this file and is compatible with recent Docker versions.
2-3: LGTM: Clear service definition.The service is well-named as 'tarmac-example', which clearly indicates its purpose. The single service definition is appropriate for a simple example or test setup.
5-6: LGTM: Appropriate port mapping.The port mapping from host port 80 to container port 8080 is appropriate. It allows easy access to the service via the standard HTTP port on the host, which is a common practice for web services.
testdata/base/fail/Makefile (2)
5-5:⚠️ Potential issueReconsider running the container as root
Adding
-u rootto the Docker run command gives the container full permissions, which violates the principle of least privilege and may pose security risks. Consider running the container with a non-root user if possible.If root permissions are necessary for specific operations, consider:
- Creating a custom user with only the required permissions.
- Using
gosuor similar tools to drop privileges after performing root-required operations.To check if the build process actually requires root permissions, you can try running without
-u root:#!/bin/bash # Attempt to build without root permissions docker run --rm -v `pwd`:/build -w /build tinygo/tinygo:latest tinygo build -o /build/tarmac.wasm -target wasi /build/main.go # Check the exit status if [ $? -eq 0 ]; then echo "Build successful without root permissions" else echo "Build failed without root permissions" fiThis will help determine if root permissions are truly necessary for the build process.
5-5:⚠️ Potential issueConsider pinning the Docker image to a specific version
While using
tinygo/tinygo:latestensures you always have the most recent version, it may introduce unexpected behavior or breaking changes in your build process. Consider pinning to a specific version (e.g.,tinygo/tinygo:0.26.0) to ensure reproducible builds.To check the current TinyGo version and available tags, you can run:
This will help you choose a specific version to pin to.
testdata/base/successafter5/Makefile (2)
Line range hint
1-4: LGTM: Unchanged parts of the MakefileThe rest of the Makefile, including the
docker-composeandruntargets, remains unchanged and appears to be correct. Theruntarget properly depends on bothbuildanddocker-compose, ensuring that the WASM file is built before running the Docker Compose setup.Also applies to: 6-11
5-5: 🛠️ Refactor suggestionConsider version pinning and security implications
The changes to the Docker command in the
buildtarget introduce both improvements and potential concerns:
Using
latestinstead of a specific TinyGo version:
- While this ensures you always use the most recent version, it may lead to unexpected behavior if there are breaking changes. Consider using a specific version and updating it deliberately.
The
--rmflag is a good addition for automatic cleanup.Running as root (
-u root):
- This solves permission issues but introduces potential security risks. Consider creating a non-root user with just enough permissions to perform the build.
Consider modifying the command to address these points:
-docker run --rm -v `pwd`:/build -w /build -u root tinygo/tinygo:latest tinygo build -o /build/tarmac.wasm -target wasi /build/main.go +docker run --rm -v `pwd`:/build -w /build -u 1000:1000 tinygo/tinygo:0.28.1 tinygo build -o /build/tarmac.wasm -target wasi /build/main.goThis suggestion:
- Pins to a specific TinyGo version (0.28.1 as an example, use the appropriate version).
- Keeps the
--rmflag for cleanup.- Uses a non-root user (UID 1000) instead of root.
To ensure the non-root user approach works, we need to verify the permissions of the current directory. Run this script:
If the script shows that the current user (likely UID 1000) has write permissions and can create/delete files, the non-root approach should work. If not, you may need to adjust permissions or use a different UID.
✅ Verification successful
Permission verification successful
The non-root user approach is valid as the current user has the necessary write permissions in the directory.
- Using
-u 1000:1000will not cause permission issues during the build process.This ensures improved security by avoiding running the container as root.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check current directory permissions ls -l . | head -n 1 # Check if the current user can write to the directory touch test_file && rm test_file echo $?Length of output: 74
testdata/base/function/Makefile (1)
5-5: 🛠️ Refactor suggestionReview Docker run command changes
The changes to the Docker run command in the build target introduce both improvements and potential concerns:
Updating to
tinygo/tinygo:latest:
While this ensures you're using the most recent version of TinyGo, it may lead to unexpected behavior or breaking changes in the future. Consider using a specific version and updating it manually to maintain better control over your build environment.Adding
--rmflag:
This is a good practice for improved container management, automatically removing the container after it exits.Adding
-u rootoption:
Running the container as root provides necessary permissions but poses potential security risks. Consider creating a non-root user with the minimum required permissions instead.Consider the following improvements:
- docker run --rm -v `pwd`/:/build -w /build -u root tinygo/tinygo:latest tinygo build -o /build/tarmac.wasm -target wasi /build/main.go + docker run --rm -v `pwd`/:/build -w /build -u $(id -u):$(id -g) tinygo/tinygo:0.28.1 tinygo build -o /build/tarmac.wasm -target wasi /build/main.goThis suggestion:
- Uses a specific TinyGo version (update to the latest stable version as needed).
- Replaces
-u rootwith-u $(id -u):$(id -g)to run the container with the current user's UID and GID, improving security.To ensure these changes don't break the build process, please run the following verification script:
testdata/base/kv/Makefile (1)
7-8: 🛠️ Refactor suggestionConsider renaming the target and adding options for better control.
The
docker-composetarget correctly uses the newerdocker composesyntax. However, there are a few points to consider:
- The target name (
docker-compose) doesn't match the command syntax (docker compose). Consider renaming the target todocker-compose-upor simplyupfor clarity.- The command lacks options that might be useful, such as detached mode or removal of orphaned containers.
Consider updating the target as follows:
up: docker compose up -d --remove-orphansThis runs containers in detached mode (
-d) and removes any orphaned containers (--remove-orphans).Could you clarify if there's a specific reason for not including these options? Also, is there a
docker-compose.ymlfile in the same directory? Let's verify:✅ Verification successful
Review suggestion is valid and improves clarity and functionality.
The
docker-composetarget is appropriately suggested to be renamed and enhanced with additional options for better control.
- Renaming to
upand adding-d --remove-orphansimproves the Makefile.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for docker-compose.yml file if [ -f docker-compose.yml ]; then echo "docker-compose.yml file found." else echo "Warning: docker-compose.yml file not found in the current directory." fiLength of output: 102
testdata/base/sql/Makefile (1)
1-10: Overall, the Makefile is well-structured and functional.The Makefile effectively manages the build and execution process for the Go example using Tarmac WASM functions. It provides clear targets for building the WASM binary, starting Docker Compose services, and a combined run target.
While the current implementation is functional, the suggested improvements focus on:
- Enhancing build consistency by pinning the TinyGo version.
- Improving error handling and environment checks.
- Adding flexibility to the run process with options to skip unnecessary steps.
- Ensuring proper Makefile practices with
.PHONYdirectives.Implementing these suggestions will result in a more robust, consistent, and user-friendly build process.
testdata/sdkv1/logger/Makefile (1)
1-10: Overall, the Makefile provides a solid foundation with room for minor enhancements.This Makefile effectively manages the build and execution process for the Tarmac WASM functions. It covers the essential tasks of building the WebAssembly binary and managing Docker containers. The use of Docker for the build process ensures consistency across different environments.
While the current implementation is functional, the suggested improvements in the previous comments will enhance its usability and maintainability:
- Using a specific TinyGo version for reproducible builds
- Renaming the
docker-composetarget for consistency- Adding options for running containers in detached mode
- Including a target to stop the containers
These minor adjustments will result in a more robust and user-friendly Makefile.
testdata/sdkv1/sql/Makefile (1)
10-10: LGTM! Efficient combination of targets.The
runtarget effectively combines thebuildanddocker-composetargets, providing a convenient way to build the WebAssembly binary and start the services with a single command. The order of execution is correct, ensuring that the binary is built before the services are started.testdata/sdkv1/logger/main.go (1)
1-8: LGTM: Clear package declaration and appropriate imports.The file starts with a clear comment explaining its purpose, which is good practice. The package is correctly declared as
mainfor an executable program, and the imports are appropriate for the functionality provided.testdata/sdkv1/kv/main.go (3)
1-9: LGTM: Package declaration and imports are correct.The file starts with a clear comment explaining its purpose as a test program for Tarmac unit testing. The package declaration and imports are appropriate for the task at hand.
41-43: LGTM: Final return statement is correct.The function correctly returns a success message as a byte slice if all operations complete successfully. This aligns with the expected behavior of a Tarmac SDK handler.
1-43: Overall assessment: Well-implemented test program with minor improvement suggestions.This test program effectively demonstrates the usage of the Tarmac SDK for key-value operations. It covers SDK initialization, data storage, retrieval, and basic validation. The code structure is clear and follows good practices.
A few minor improvements have been suggested:
- Enhanced error handling in the
mainfunction.- Clarification on the unused
payloadparameter in theHandlerfunction.- More informative error message for data validation.
These changes will improve debugging and make the code more robust. Great job on implementing this test program!
testdata/tarmac.json (5)
7-7: LGTM: Filepath updated correctly for 'default' functionThe filepath for the 'default' function has been updated to include the new 'base' directory. This change is consistent with the overall restructuring mentioned in the PR objectives.
10-10: LGTM: Filepath updated correctly for 'kv' functionThe filepath for the 'kv' function has been updated to include the new 'base' directory. This change is consistent with the overall restructuring mentioned in the PR objectives.
14-14: LGTM: Filepath updated correctly for 'logger' functionThe filepath for the 'logger' function has been updated to include the new 'base' directory. This change is consistent with the overall restructuring mentioned in the PR objectives.
17-17: LGTM: Filepath updated correctly for 'sql' functionThe filepath for the 'sql' function has been updated to include the new 'base' directory. This change is consistent with the overall restructuring mentioned in the PR objectives.
7-21: Overall change looks good, but verify system-wide impactsThe filepath updates for all functions are consistent and align with the PR objectives of restructuring. However, it's crucial to ensure that these changes are reflected in any part of the system that interacts with these WASM modules.
To confirm that these changes are properly handled throughout the codebase, please run the following script:
This script will help identify any instances where the old paths might still be in use, and confirm that the new base directory is properly referenced in relevant files.
testdata/sdkv1/tarmac.json (3)
1-5: Service definition looks good.The service "test-service" is properly defined at the root of the JSON structure.
1-72: Overall, the configuration file is well-structured and comprehensive.This new
tarmac.jsonfile provides a robust test configuration for the "test-service". It effectively demonstrates various features including different function types, HTTP routes, scheduled tasks, and direct function invocation. The structure is clear and follows proper JSON formatting.A few minor suggestions were made:
- Verify the filepath for the "func" function.
- Review the usage of the pool_size property across functions.
- Consider adding descriptions to routes for improved clarity.
These enhancements would further improve the already solid configuration file.
5-24: Functions are well-defined, but consider reviewing a few points.The functions section is structured correctly, with each function having the necessary filepath property.
However, there are a couple of points to consider:
The filepath for the "func" function is in a different directory (/testdata/base/) compared to the others. Please verify if this is intentional.
The pool_size property is only set for some functions (kv, sql, func). Consider if this property should be consistently applied to all functions or if the current configuration is intentional.
To verify the filepath for the "func" function, you can run the following command:
This command will search for the tarmac.wasm file in the specified directory. If the file exists, it confirms that the filepath is correct.
pkg/wasm/wasm_test.go (4)
40-40: LGTM: File path updated consistentlyThe update to the file path from "/testdata/logger/tarmac.wasm" to "/testdata/base/logger/tarmac.wasm" appears to be a consistent change in the directory structure. This modification is applied uniformly across all test cases, maintaining consistency in the test setup.
50-50: LGTM: Consistent file path updateThe file path update in this test case is consistent with the previous change, maintaining the new directory structure "/testdata/base/logger/tarmac.wasm".
60-60: LGTM: Consistent file path updatesThe file path updates in these test cases are consistent with the previous changes:
- Line 60: Updated to "/testdata/base/logger/tarmac.wasm" for the "No Pool Size" test case.
- Line 81: Updated to "/doesntexist/testdata/base/logger/tarmac.wasm" for the "Bad File Location" test case, maintaining its purpose while reflecting the new directory structure.
These changes ensure consistency across all test cases while preserving the intended functionality of each test.
Also applies to: 81-81
Line range hint
1-150: Summary: File path updates are consistent and maintain test integrityThe changes in this file consistently update the WASM file paths across all test cases in both
TestWASMModuleCreationandTestWASMExecutionfunctions. These updates reflect a change in the directory structure from "/testdata/logger/tarmac.wasm" to "/testdata/base/logger/tarmac.wasm".Key points:
- All test cases have been updated consistently.
- The "Bad File Location" test case maintains its purpose while reflecting the new structure.
- The changes do not alter the logic or intent of the tests.
The suggested improvement to use a constant for the file path could enhance maintainability for future updates.
Overall, these changes appear to be part of a broader refactoring to support the new directory structure, and they have been implemented correctly in this file.
pkg/callbacks/httpclient/httpclient.go (3)
31-34: LGTM: New imports for protobuf supportThe new imports for protobuf support are correctly added and necessary for the implementation.
93-125: LGTM: HTTP request execution and response handlingThe implementation of HTTP request execution and response handling is correct and consistent. It properly sets user-supplied headers and populates the
HTTPClientResponseprotobuf message with the response data.Note: There's an existing comment about limiting the response body size to prevent excessive memory usage. This suggestion is still valid and should be addressed in a follow-up task.
127-138: LGTM: Response marshaling and returnThe response marshaling using protobuf and the error handling for non-200 status codes are implemented correctly.
pkg/callbacks/sql/sql.go (6)
27-30: LGTM: New imports for protobuf supportThe addition of protobuf-related imports is appropriate for the new functionality being introduced.
60-117: LGTM: New Exec method for SQL command executionThe new
Execmethod is well-implemented with comprehensive error handling and result processing. It correctly uses protobuf for input and output, and the use of status code 206 for partial content is consistent with the project's conventions.
123-169: LGTM: Updated Query method with protobuf supportThe changes to the
Querymethod successfully introduce protobuf support while maintaining backward compatibility with JSON. The fallback mechanism and comprehensive error handling are well-implemented.
199-213: LGTM: Refactored queryJSON methodThe
queryJSONmethod has been successfully refactored to use the newqueryhelper function, improving code reusability while maintaining backward compatibility with the existing JSON interface.
274-277: LGTM: New exec helper methodThe new
exechelper method provides a simple way to execute SQL commands. As per the project's design and use case, it's acceptable to execute SQL queries directly without using parameterized queries in this context.
Line range hint
1-277: Overall: Excellent implementation of protobuf support with backward compatibilityThe changes in this file successfully introduce Protocol Buffers support while maintaining backward compatibility with the existing JSON interface. The new helper methods (
queryandexec) improve code organization and reusability. The error handling and result processing have been enhanced throughout the file, leading to more robust and maintainable code.The gradual transition from JSON to protobuf, with fallback mechanisms in place, allows for a smooth migration process for existing clients. This approach aligns well with the PR objectives of transitioning to Protobuf-based messaging while ensuring that existing functionalities are not disrupted for users relying on JSON.
Great job on implementing these significant changes while keeping the code clean and maintainable!
pkg/callbacks/kvstore/kvstore.go (5)
31-33: LGTM: New imports and error variableThe addition of protobuf imports and the new
ErrNilKeyerror variable are appropriate for the transition to protobuf support while maintaining a clean error handling approach.Also applies to: 55-58
153-196: LGTM: Updated Set method with protobuf supportThe
Setmethod has been successfully updated to support both protobuf and JSON inputs, maintaining consistency with theGetmethod. The error handling is comprehensive, including checks for nil key and data, and appropriate use of status codes.
242-280: LGTM: Updated Delete method with protobuf supportThe
Deletemethod has been successfully updated to support both protobuf and JSON inputs, maintaining consistency with theGetandSetmethods. The error handling is comprehensive, including a check for nil key, and appropriate use of status codes.
113-115: LGTM: Retained JSON-based methods for backward compatibilityThe retention of JSON-based methods (
getJSON,setJSON,deleteJSON,keysJSON) as fallbacks is an excellent approach for ensuring backward compatibility. This allows for a gradual transition to protobuf while maintaining support for existing JSON-based clients.Also applies to: 198-200, 282-284, 350-350
Line range hint
1-373: Overall LGTM: Successful integration of protobuf supportThe changes in this file successfully integrate protobuf support while maintaining backward compatibility with JSON. The consistent approach across different methods (
Get,Set,Delete,Keys) enhances maintainability. The introduction ofErrNilKeyand improved error handling contribute to more robust code.Great job on implementing these significant changes while keeping the overall structure of the package consistent and backward-compatible!
pkg/callbacks/sql/sql_test.go (4)
15-16: LGTM: New imports for Protobuf supportThe new imports for Protobuf support have been correctly added and use clear, concise naming conventions.
68-227: LGTM: Well-structured Protobuf-based testsThe new Protobuf-based tests are well-structured, covering both happy and unhappy paths. They mirror the existing JSON tests, which maintains consistency in the test suite. The tests adequately cover the main CRUD operations and error scenarios.
Line range hint
1-383: Overall improvement in test coverage and structureThe changes to this file significantly enhance the test suite by adding Protobuf-based tests alongside the existing JSON-based tests. The new structure improves organization and readability, while maintaining consistency between the two test types. The comprehensive coverage of both happy and unhappy paths for CRUD operations is commendable.
A few minor issues were identified:
- Typos in error messages (lines 81 and 237)
- Inconsistent error messages across different tests
Addressing these minor issues will further improve the quality of the test suite. Overall, these changes represent a substantial improvement in the robustness and coverage of the SQL package tests.
81-81:⚠️ Potential issueFix typo in error message
There's a typo in the error message. "Unexepected" should be corrected to "Unexpected" to ensure clarity in test failure messages.
Apply this diff to fix the typo:
- t.Fatalf("Unexepected success with failure test case") + t.Fatalf("Unexpected success with failure test case")Likely invalid or redundant comment.
pkg/callbacks/metrics/metrics_test.go (5)
5-7: LGTM: Appropriate imports for Protobuf integrationThe addition of Protobuf-related imports aligns well with the PR objective of migrating to Protobuf for callbacks. The imports are correctly placed and aliased for clarity.
Line range hint
10-18: LGTM: Improved MetricsCase struct for more precise testingThe modifications to the MetricsCase struct, including the removal of the JSON field and the addition of Action and Value fields, allow for more structured and specific test case definitions. This change aligns well with the transition from JSON to Protobuf and enables more precise testing scenarios.
21-256: LGTM: Comprehensive test coverage with Protobuf integrationThe TestMetricsCallback function has been significantly enhanced:
- Test cases have been expanded to cover a wide range of scenarios for different metric types (counter, gauge, histogram).
- The function now uses Protobuf messages instead of JSON, aligning with the PR objective.
- The structure of the test function remains clear and readable despite the additions.
These changes provide more robust testing of the metrics callback functionality while successfully integrating Protobuf.
258-265: LGTM: New MetricsCaseJSON struct for backward compatibilityThe addition of the MetricsCaseJSON struct is a good approach for maintaining backward compatibility with JSON-based test cases. This structure allows for clear definition of JSON-based test scenarios while the system transitions to Protobuf.
Line range hint
267-495: LGTM: Comprehensive JSON callback testing for backward compatibilityThe new TestMetricsCallbackJSON function is a valuable addition:
- It provides comprehensive testing for JSON-based callbacks, maintaining backward compatibility.
- The test cases cover various scenarios, including error cases and edge cases.
- The structure is consistent with TestMetricsCallback, maintaining readability and consistency in the test suite.
This addition ensures that both Protobuf and JSON-based callbacks are thoroughly tested, supporting a smooth transition between the two formats.
pkg/app/server_test.go (5)
98-98: LGTM. Consistent path update.The WASM function path update is consistent with the change in TestBasicFunction.
403-403: LGTM. Consistent path update in TestWASMRunner.The WASM function path update in TestWASMRunner is consistent with the changes in other test functions.
Line range hint
1-463: Overall LGTM. Consistent updates and improved test coverage.The changes in this file can be summarized as follows:
- Consistent updates to WASM function paths across all test functions, indicating a restructuring of the test data directory.
- Addition of new test cases for SDKv1 compatibility, improving test coverage for different SDK versions.
These changes enhance the test suite by ensuring it works with the new directory structure and covers multiple SDK versions. The consistency of the changes suggests a systematic approach to updating the tests.
To ensure all changes are correct and consistent, please run the entire test suite:
#!/bin/bash # Run the entire test suite go test ./... -v # Check if any tests failed if [ $? -ne 0 ]; then echo "Some tests failed. Please review the output above." exit 1 else echo "All tests passed successfully." fi
314-314: LGTM. Consistent updates to WASM function paths in configurations.The WASM function paths in the JSON configurations have been updated consistently across all test cases in TestInitFuncs.
Please verify the existence of all updated WASM function files:
Also applies to: 324-324, 335-335, 345-345
29-29: LGTM. Verify the new WASM function path.The WASM function path has been updated to include a "base" directory. This change appears to be part of a broader restructuring of test data.
Please ensure that the new path
/testdata/base/default/tarmac.wasmis correct and the file exists.pkg/app/app_test.go (5)
176-176: LGTM. Path update is consistent.The update to the
wasm_functionpath in this test function is consistent with the changes observed in other parts of the file.
224-224: LGTM. Path update maintains consistency.The
wasm_functionpath update in this test function aligns with the changes made throughout the file, maintaining consistency across all test cases.
403-403: LGTM. Ensure mTLS functionality with updated path.The
wasm_functionpath update aligns with the changes made throughout the file. As this test involves mTLS configuration, it's crucial to verify that the mTLS functionality remains intact with the new path.To confirm that the mTLS functionality is working correctly with the updated WASM function path, execute the following command:
#!/bin/bash # Verify mTLS connection and HTTPS response response=$(curl -k --cert /tmp/cert --key /tmp/key -s -o /dev/null -w "%{http_code}" https://localhost:9000/health) if [ "$response" = "200" ]; then echo "mTLS connection successful and received 200 OK response." else echo "mTLS connection failed or received unexpected response: $response" fi
294-294: LGTM. Verify TLS functionality with updated path.The
wasm_functionpath update is consistent with previous changes. Since this test involves TLS configuration, it's important to ensure that the TLS functionality remains unaffected by the path change.To verify that the TLS functionality is working correctly with the updated WASM function path, run the following command:
#!/bin/bash # Verify TLS connection and HTTPS response response=$(curl -k -s -o /dev/null -w "%{http_code}" https://localhost:9000/health) if [ "$response" = "200" ]; then echo "TLS connection successful and received 200 OK response." else echo "TLS connection failed or received unexpected response: $response" fi
507-507: LGTM. Verify failing mTLS scenario with updated path.The
wasm_functionpath update is consistent with the changes throughout the file. As this test is designed to verify a failing mTLS scenario, it's important to ensure that this negative test case still functions as expected with the new path.To confirm that the failing mTLS scenario still works as intended with the updated WASM function path, run the following command:
#!/bin/bash # Verify that the mTLS connection fails as expected response=$(curl -k -s -o /dev/null -w "%{http_code}" https://localhost:9000/health) if [ "$response" = "000" ]; then echo "mTLS connection failed as expected (connection refused)." else echo "Unexpected response received: $response. The failing mTLS scenario may not be working as intended." fitestdata/base/sql/main.go (1)
27-27: Verify the SQL query for compatibility with your databaseEnsure that the SQL query provided is compatible with the target database's SQL dialect. For example, some databases might require
INTto be uppercase or have different syntax for specifyingPRIMARY KEY.Run the following script to check the SQL query against known syntax patterns:
This script helps identify any immediate syntax issues in the query.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Added a delightful bouquet of tests for SQLExec, including your standard 'Happy Path' stroll, an 'Unhappy Path' stumble, and the 'Empty Exec' shrug. Who knew SQL testing could be such an emotional rollercoaster?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/callbacks/sql/sql_test.go (2)
99-224: LGTM with suggestion: Protobuf-based Happy Path testsThe Happy Path tests for Protobuf-based queries are comprehensive and well-implemented. They cover essential SQL operations and properly verify the results.
However, consider using
db.Execinstead ofdb.Queryfor non-SELECT operations in the "Create Table" and "Delete Table" tests. This would be more appropriate for statements that don't return rows.Consider applying this change to the "Create Table" and "Delete Table" tests:
- r, err := db.Query(qMsg) + r, err := db.Exec(qMsg)This change would make the usage consistent with the "Insert Data" test and follow best practices for SQL operations.
306-457: LGTM with minor suggestion: JSON-based query testsThe JSON-based query tests are well-maintained for backward compatibility. They cover both Unhappy Path and Happy Path scenarios, ensuring that the existing functionality remains intact.
However, there's a minor typo in an error message that should be corrected.
Fix the typo in the error message on line 314:
- t.Fatalf("Unexepected success with failure test case") + t.Fatalf("Unexpected success with failure test case")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/callbacks/sql/sql_test.go (3 hunks)
🧰 Additional context used
📓 Learnings (1)
pkg/callbacks/sql/sql_test.go (2)
Learnt from: madflojo PR: tarmac-project/tarmac#137 File: pkg/callbacks/sql/sql_test.go:107-107 Timestamp: 2024-10-13T16:47:26.151Z Learning: In `pkg/callbacks/sql/sql_test.go`, within the "Create Table" and "Delete Table" tests, using `db.Query` for non-SELECT statements is acceptable, as various scenarios are being intentionally tested.Learnt from: madflojo PR: tarmac-project/tarmac#137 File: pkg/callbacks/sql/sql_test.go:109-109 Timestamp: 2024-10-13T17:26:09.118Z Learning: In `pkg/callbacks/sql/sql_test.go`, it's preferred to keep specific error messages in tests to better understand the cause of failures.
🔇 Additional comments (3)
pkg/callbacks/sql/sql_test.go (3)
15-16: LGTM: New imports for Protobuf supportThe addition of Protobuf-related imports is appropriate for the changes made in this file. These imports enable the use of Protobuf messages for SQL operations.
68-97: LGTM: Protobuf-based Unhappy Path testsThe implementation of Unhappy Path tests for Protobuf-based queries is well-structured and comprehensive. It correctly uses Protobuf messages, executes queries, and verifies error handling and status codes.
Line range hint
229-303: LGTM: SQLExec testsThe SQLExec tests are well-implemented, covering both Happy Path and Unhappy Path scenarios. They correctly use Protobuf messages, execute SQL statements, and verify the responses. The error handling and status code checks are appropriate.
This pull request aims to migrate from JSON messaging between WASM Modules and the Host to Protobuf-based messaging. Of course, we will still support JSON to avoid breaking anyone.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests