chore: deprecate old profiletypes in favor of containerprofiles - #168
Open
entlein wants to merge 5 commits into
Open
chore: deprecate old profiletypes in favor of containerprofiles#168entlein wants to merge 5 commits into
entlein wants to merge 5 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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughContainerProfile replaces ApplicationProfile and NetworkNeighborhood in configured in-cluster synchronization. Typed routing, client behavior tests, filtering fixtures, dependencies, and end-to-end Kubernetes, storage, Postgres, and S3 scenarios are updated. ChangesContainerProfile synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Kubernetes
participant InClusterClient
participant StorageClient
participant Postgres
participant S3
Kubernetes->>InClusterClient: Create or update ContainerProfile
InClusterClient->>StorageClient: List, watch, or get ContainerProfile
InClusterClient->>Postgres: Write synchronization metadata
InClusterClient->>S3: Write ContainerProfile payload
InClusterClient->>Kubernetes: Reconcile or delete ContainerProfile
🚥 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 |
This was referenced Jul 30, 2026
added 4 commits
July 30, 2026 21:21
The ApplicationProfile/NetworkNeighborhood CRDs are removed upstream, so drop their cases from chooseLister/chooseWatcher/getResource and the client config; the synchronizer watches and ships ContainerProfile only. (Dead AP/NN test fixtures remain until the storage dependency is bumped past the removal.) Signed-off-by: entlein <einentlein@gmail.com>
The in-cluster watch config was migrated to containerprofiles but the config test's expected value still asserted applicationprofiles. Signed-off-by: entlein <einentlein@gmail.com>
The non-test synchronizer already syncs only containerprofiles; the tests still referenced the removed ApplicationProfile / NetworkNeighborhood types and would not compile against the migrated storage. Move the filter and integration tests (and the filtered-output testdata) to ContainerProfile, mapping the old NetworkNeighborhood per-container egress onto the flat ContainerProfile spec. Pin storage v0.0.290. Signed-off-by: entlein <einentlein@gmail.com>
Give the CP sync path a local safety net (previously covered only by the separate-module, private-dep, cluster-required integration suite): routing of the containerprofiles resource through chooseLister/chooseWatcher/getResource, the reconcile/put/patch/delete flow via fake dynamic+storage clients, and watchRetry's CP branch via the fake storage watcher (no production seam needed). Signed-off-by: entlein <einentlein@gmail.com>
entlein
marked this pull request as ready for review
July 30, 2026 19:21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
adapters/incluster/v1/client_cp_test.go (1)
460-502: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueIntentional long-lived goroutines in
TestCP_WatchRetry_ViaFakeStorageWatcher.Both the
watchRetrygoroutine and the CP-creation loop keep running for the remainder of the test binary's process life (never canceled/closed), per the documented tradeoff to avoid theos.Exit(1)giveup path. Locally scoped and non-blocking for other tests, but worth keeping in mind if this pattern is reused elsewhere in the suite.🤖 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 `@adapters/incluster/v1/client_cp_test.go` around lines 460 - 502, The test leaves both the watchRetry goroutine and CP-creation loop running beyond the test scope. Update TestCP_WatchRetry_ViaFakeStorageWatcher to use cancellable context and coordinated cleanup, ensuring goroutines exit when the test completes while avoiding the watchRetry os.Exit(1) give-up path.
🤖 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 `@tests/synchronizer_integration_test.go`:
- Around line 1295-1302: Handle the error returned by
ContainerProfiles(namespace).Create immediately in the setup before calling
waitForObjectInPostgres or json.Marshal. Fail the test through the existing
testing mechanism when creation fails, preserving createdContainerProfileObj for
subsequent serialization only after successful creation.
---
Nitpick comments:
In `@adapters/incluster/v1/client_cp_test.go`:
- Around line 460-502: The test leaves both the watchRetry goroutine and
CP-creation loop running beyond the test scope. Update
TestCP_WatchRetry_ViaFakeStorageWatcher to use cancellable context and
coordinated cleanup, ensuring goroutines exit when the test completes while
avoiding the watchRetry os.Exit(1) give-up path.
🪄 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: b03df2bd-3bc4-4f90-8cbe-fd170905ee19
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
adapters/incluster/v1/client.goadapters/incluster/v1/client_cp_test.goadapters/incluster/v1/client_test.goadapters/incluster/v1/testdata/containerProfileFiltered.jsonadapters/incluster/v1/testdata/networkNeighborhoodFiltered.jsonconfig/config_test.goconfiguration/client/config.jsongo.modtests/synchronizer_integration_test.go
💤 Files with no reviewable changes (1)
- adapters/incluster/v1/testdata/networkNeighborhoodFiltered.json
Signed-off-by: entlein <einentlein@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Summary by CodeRabbit
New Features
Bug Fixes
Tests