Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @alpe's task in 1m 15s —— View job Code Review
SummaryThe benchmarks are well-structured and follow good Go benchmarking practices. A few issues to address: Issues1. Copy-pasted comments point to the wrong provider (both files)
// export EVNODE_E2E_GCP_KMS_KEY_NAME=projects/<project-id>/...
// export EVNODE_E2E_GCP_KMS_CREDENTIALS_FILE=...
// go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/gcpBut the file is in package Similarly, 2. GCP timeout is 1s vs AWS's 10s (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3205 +/- ##
=======================================
Coverage 61.42% 61.42%
=======================================
Files 120 120
Lines 12449 12449
=======================================
Hits 7647 7647
Misses 3942 3942
Partials 860 860
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughTwo new benchmark test files added for AWS and GCP KMS signers. Each benchmark reads cloud provider configuration from environment variables, creates a KMS signer with specified timeouts, marshals protobuf header data, and measures signing operation performance across multiple iterations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/signer/aws/signer_benchmark_test.go (1)
60-83: Consider extracting shared test helpers.The
benchmarkHeaderDatafunction is duplicated between the AWS and GCP benchmark files. This is acceptable for test isolation, but if more signer implementations are added, consider extracting this to a sharedpkg/signer/testutilpackage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signer/aws/signer_benchmark_test.go` around lines 60 - 83, Extract the duplicated benchmarkHeaderData helper into a shared test utility package (e.g., package testutil under pkg/signer/testutil) and update both AWS and GCP benchmark files to call the shared helper; specifically, create a function (preferably func BenchmarkHeaderData(tb testing.TB) []byte) that contains the proto.Header construction and proto.Marshal logic currently in benchmarkHeaderData, import pkg/signer/testutil in signer_benchmark_test.go (and the GCP counterpart) and replace calls to the local benchmarkHeaderData(b) with testutil.BenchmarkHeaderData(b), and ensure necessary imports (testing, proto, require) are present in the new testutil package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/signer/aws/signer_benchmark_test.go`:
- Around line 15-18: The comment block in signer_benchmark_test.go incorrectly
references GCP (env vars and package path) for the BenchmarkKmsSignerSign
benchmark; update those comments to show the AWS equivalents and package path
(e.g., replace GCP env var examples and "pkg/signer/gcp" with the AWS
environment variables used by the test and "pkg/signer/aws") so the doc comment
matches the AWS test implementation (refer to BenchmarkKmsSignerSign in this
file to verify the correct AWS variable names).
In `@pkg/signer/gcp/signer_benchmark_test.go`:
- Around line 15-18: The benchmark comment block in signer_benchmark_test.go
wrongly references AWS (EVNODE_E2E_AWS_KMS_KEY_ID, EVNODE_E2E_AWS_KMS_REGION and
./pkg/signer/aws); update that comment to GCP-specific environment variables and
path used by the GCP benchmark (e.g., replace AWS env var names with the GCP KMS
equivalents your test expects and change the package path to ./pkg/signer/gcp)
so the comment matches the actual BenchmarkKmsSignerSign implementation and
required GCP setup.
---
Nitpick comments:
In `@pkg/signer/aws/signer_benchmark_test.go`:
- Around line 60-83: Extract the duplicated benchmarkHeaderData helper into a
shared test utility package (e.g., package testutil under pkg/signer/testutil)
and update both AWS and GCP benchmark files to call the shared helper;
specifically, create a function (preferably func BenchmarkHeaderData(tb
testing.TB) []byte) that contains the proto.Header construction and
proto.Marshal logic currently in benchmarkHeaderData, import pkg/signer/testutil
in signer_benchmark_test.go (and the GCP counterpart) and replace calls to the
local benchmarkHeaderData(b) with testutil.BenchmarkHeaderData(b), and ensure
necessary imports (testing, proto, require) are present in the new testutil
package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c73fda29-18db-4c27-9f3b-41e7d10aadb6
📒 Files selected for processing (2)
pkg/signer/aws/signer_benchmark_test.gopkg/signer/gcp/signer_benchmark_test.go
| // BenchmarkKmsSignerSign test kms round trip | ||
| // export EVNODE_E2E_GCP_KMS_KEY_NAME=projects/<project-id>/locations/<region>/keyRings/<keyring-name>/cryptoKeys/<key-name>/cryptoKeyVersions/1 | ||
| // export EVNODE_E2E_GCP_KMS_CREDENTIALS_FILE=... | ||
| // go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/gcp |
There was a problem hiding this comment.
Copy-paste error: comments describe GCP instead of AWS.
The comment block references GCP environment variables and the GCP package path, but this is the AWS benchmark file. The actual code correctly uses AWS variables.
📝 Suggested fix
// BenchmarkKmsSignerSign test kms round trip
-// export EVNODE_E2E_GCP_KMS_KEY_NAME=projects/<project-id>/locations/<region>/keyRings/<keyring-name>/cryptoKeys/<key-name>/cryptoKeyVersions/1
-// export EVNODE_E2E_GCP_KMS_CREDENTIALS_FILE=...
-// go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/gcp
+// export EVNODE_E2E_AWS_KMS_KEY_ID="arn:aws:kms:...."
+// export EVNODE_E2E_AWS_KMS_REGION=eu-west-1
+// go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/aws📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // BenchmarkKmsSignerSign test kms round trip | |
| // export EVNODE_E2E_GCP_KMS_KEY_NAME=projects/<project-id>/locations/<region>/keyRings/<keyring-name>/cryptoKeys/<key-name>/cryptoKeyVersions/1 | |
| // export EVNODE_E2E_GCP_KMS_CREDENTIALS_FILE=... | |
| // go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/gcp | |
| // BenchmarkKmsSignerSign test kms round trip | |
| // export EVNODE_E2E_AWS_KMS_KEY_ID="arn:aws:kms:...." | |
| // export EVNODE_E2E_AWS_KMS_REGION=eu-west-1 | |
| // go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/aws |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/signer/aws/signer_benchmark_test.go` around lines 15 - 18, The comment
block in signer_benchmark_test.go incorrectly references GCP (env vars and
package path) for the BenchmarkKmsSignerSign benchmark; update those comments to
show the AWS equivalents and package path (e.g., replace GCP env var examples
and "pkg/signer/gcp" with the AWS environment variables used by the test and
"pkg/signer/aws") so the doc comment matches the AWS test implementation (refer
to BenchmarkKmsSignerSign in this file to verify the correct AWS variable
names).
| // BenchmarkKmsSignerSign test kms round trip | ||
| // export EVNODE_E2E_AWS_KMS_KEY_ID="arn:aws:kms:...." | ||
| // export EVNODE_E2E_AWS_KMS_REGION=eu-west-1 | ||
| // go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/aws |
There was a problem hiding this comment.
Copy-paste error: comments describe AWS instead of GCP.
The comment block references AWS environment variables and the AWS package path, but this is the GCP benchmark file. The actual code correctly uses GCP variables.
📝 Suggested fix
// BenchmarkKmsSignerSign test kms round trip
-// export EVNODE_E2E_AWS_KMS_KEY_ID="arn:aws:kms:...."
-// export EVNODE_E2E_AWS_KMS_REGION=eu-west-1
-// go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/aws
+// export EVNODE_E2E_GCP_KMS_KEY_NAME=projects/<project-id>/locations/<region>/keyRings/<keyring-name>/cryptoKeys/<key-name>/cryptoKeyVersions/1
+// export EVNODE_E2E_GCP_KMS_CREDENTIALS_FILE=...
+// go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/gcp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // BenchmarkKmsSignerSign test kms round trip | |
| // export EVNODE_E2E_AWS_KMS_KEY_ID="arn:aws:kms:...." | |
| // export EVNODE_E2E_AWS_KMS_REGION=eu-west-1 | |
| // go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/aws | |
| // BenchmarkKmsSignerSign test kms round trip | |
| // export EVNODE_E2E_GCP_KMS_KEY_NAME=projects/<project-id>/locations/<region>/keyRings/<keyring-name>/cryptoKeys/<key-name>/cryptoKeyVersions/1 | |
| // export EVNODE_E2E_GCP_KMS_CREDENTIALS_FILE=... | |
| // go test -v -bench=BenchmarkKmsSignerSign -benchtime=3s -count=10 -run='^$' ./pkg/signer/gcp |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/signer/gcp/signer_benchmark_test.go` around lines 15 - 18, The benchmark
comment block in signer_benchmark_test.go wrongly references AWS
(EVNODE_E2E_AWS_KMS_KEY_ID, EVNODE_E2E_AWS_KMS_REGION and ./pkg/signer/aws);
update that comment to GCP-specific environment variables and path used by the
GCP benchmark (e.g., replace AWS env var names with the GCP KMS equivalents your
test expects and change the package path to ./pkg/signer/gcp) so the comment
matches the actual BenchmarkKmsSignerSign implementation and required GCP setup.
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
utACK because i don't have such keys.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
GCP:
AWS:
Summary by CodeRabbit