chore: deprecate old profiletypes in favor of containerprofiles - #55
chore: deprecate old profiletypes in favor of containerprofiles#55entlein wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates Go dependencies, removes application and network profile storage RPCs and client methods, narrows ChangesDependency refresh
Container profile storage API
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
The ApplicationProfile/NetworkNeighborhood CRDs are removed upstream. Remove the GetApplicationProfile/GetNetworkNeighborhood/ListApplicationProfiles/ ListNetworkNeighborhoods client wrappers (no callers) and the tests covering them. The mock's AP/NN methods stay only to satisfy the still-generated proto StorageServiceClient interface; dropping the AP/NN RPCs and adding a ListContainerProfiles RPC is a storage-side proto follow-up. Signed-off-by: entlein <einentlein@gmail.com>
…roto The storage migration removed the ApplicationProfile and NetworkNeighborhood types, so the gRPC storage-service proto that embedded them no longer compiles against the migrated storage. Drop them from the surface: remove the ListApplicationProfiles / ListNetworkNeighborhoods rpcs and their request and response messages, and the application_profile / network_neighborhood fields from GetProfileResponse (reserved 4, 5). ContainerProfile is the only profile type. Regenerate the message + gRPC bindings from the updated .proto, drop the corresponding mock methods from the client test, and pin storage v0.0.290. Signed-off-by: entlein <einentlein@gmail.com>
Close the coverage gaps on the storage gRPC client: GetContainerProfileStream / SendContainerProfileStream error branches (server !Success, nil metadata, unmarshal failure, mid-stream send/recv abort), the >4 MiB chunk-split proof (client must split rather than ship one oversized frame), real bufconn round-trips for the previously mock-only unary GetContainerProfile / SendContainerProfile (incl. the not-found !Success path), and the Connect/Close/IsConnected/GetAddress dial lifecycle. Coverage 68.0% -> 71.7%. Test-only; extends the existing bufconn harness with error-injection knobs. Signed-off-by: entlein <einentlein@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/client/v1/storageclient_test.go (2)
253-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: redundant
tt := ttand stale test name.Line 255 re-declares
ttinside the subtest closure, which shadows nothing useful — the classic capture workaround belongs beforet.Run, and it is unnecessary altogether on Go 1.22+. Also, the test now exercisesGetContainerProfile, soTestStorageClient_GetProfileis a misleading name.♻️ Suggested cleanup
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt := tt mockClient := &mockStorageServiceClient{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/v1/storageclient_test.go` around lines 253 - 273, Remove the redundant inner tt := tt capture from the t.Run closure, relying on the project’s Go version semantics, and rename the enclosing test from TestStorageClient_GetProfile to accurately reflect its GetContainerProfile coverage.
582-596: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAbort path records chunk count but not received bytes.
The abort branch sets
cpReceivedChunkswhile leavingcpReceivedBytesunchanged; there are no mid-stream abort tests that would catch this so far. Also,fmtis already imported in this test file, sofmt.Errorfis not an issue.♻️ Optional: record bytes on abort too
if abort { s.mu.Lock() + s.cpReceivedBytes = buf s.cpReceivedChunks = count s.mu.Unlock()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/v1/storageclient_test.go` around lines 582 - 596, Update the abort branch in the stream handler to assign the accumulated buf to cpReceivedBytes before returning the error, while preserving the existing cpReceivedChunks update and synchronization.pkg/client/v1/proto/storage_service.proto (1)
122-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReserving 4 and 5 is right; the
kinddoc is now stale.
GetProfileRequest.kind(Line 95) still documents"applicationProfile"and"networkNeighborhood"as accepted values, but the response can no longer carry them. Worth updating that comment (and the generated bindings) in the same change so the contract reads consistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/v1/proto/storage_service.proto` around lines 122 - 126, Update the documentation for GetProfileRequest.kind to remove applicationProfile and networkNeighborhood and describe only the currently supported kind values, consistent with the response fields and reserved numbers. Regenerate the protobuf bindings so the generated API documentation reflects the updated contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 20: Update the direct google.golang.org/grpc dependency in go.mod from
v1.80.0 to the fixed release addressing GHSA-hrxh-6v49-42gf / GO-2026-6061, then
rerun vulnerability scanning. If the scanner still requires an exception,
document that the affected service/server paths are not used in production.
---
Nitpick comments:
In `@pkg/client/v1/proto/storage_service.proto`:
- Around line 122-126: Update the documentation for GetProfileRequest.kind to
remove applicationProfile and networkNeighborhood and describe only the
currently supported kind values, consistent with the response fields and
reserved numbers. Regenerate the protobuf bindings so the generated API
documentation reflects the updated contract.
In `@pkg/client/v1/storageclient_test.go`:
- Around line 253-273: Remove the redundant inner tt := tt capture from the
t.Run closure, relying on the project’s Go version semantics, and rename the
enclosing test from TestStorageClient_GetProfile to accurately reflect its
GetContainerProfile coverage.
- Around line 582-596: Update the abort branch in the stream handler to assign
the accumulated buf to cpReceivedBytes before returning the error, while
preserving the existing cpReceivedChunks update and synchronization.
🪄 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 Plus
Run ID: 195220f6-3b10-471a-83c6-2482e38e9d17
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/client/v1/proto/storage_service.pb.gois excluded by!**/*.pb.gopkg/client/v1/proto/storage_service_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
go.modpkg/client/v1/proto/storage_service.protopkg/client/v1/storageclient.gopkg/client/v1/storageclient_coverage_test.gopkg/client/v1/storageclient_test.go
💤 Files with no reviewable changes (1)
- pkg/client/v1/storageclient.go
Signed-off-by: entlein <einentlein@gmail.com>
The ApplicationProfile/NetworkNeighborhood CRDs are removed upstream. Remove the
GetApplicationProfile/GetNetworkNeighborhood/ListApplicationProfiles/
ListNetworkNeighborhoods client wrappers (no callers) and the tests covering
them. The mock's AP/NN methods stay only to satisfy the still-generated proto
StorageServiceClient interface; dropping the AP/NN RPCs and adding a
ListContainerProfiles RPC is a storage-side proto follow-up.
Summary by CodeRabbit
Changes
Improvements
Chores