OCPBUGS-78480: add new watchlist semantic support to project watcher#633
OCPBUGS-78480: add new watchlist semantic support to project watcher#633ricardomaraschini wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@ricardomaraschini: This pull request references Jira Issue OCPBUGS-78480, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughWatch now propagates an explicit sendInitialEvents flag from REST into the user project watcher; when enabled the watcher emits initial project Add events and then a Bookmark event annotated with metav1.InitialEventsAnnotationKey before normal streaming resumes. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant REST as REST.Watch
participant Watcher as UserProjectWatcher
participant Cache as AuthCache
participant Stream as EventStream
Client->>REST: Watch(options { ResourceVersion, SendInitialEvents })
REST->>Watcher: NewUserProjectWatcher(includeAllExistingProjects?, sendBookmark?)
Watcher->>Cache: enumerate existing projects (if includeAllExistingProjects)
alt includeAllExistingProjects true
Watcher->>Stream: emit watch.Added events (initial projects)
alt sendBookmark true
Watcher->>Stream: emit watch.Bookmark (InitialEventsAnnotationKey="true")
end
else includeAllExistingProjects false
alt sendBookmark true
Watcher->>Stream: emit watch.Bookmark (InitialEventsAnnotationKey="true")
end
end
Watcher->>Stream: enter main event loop -> emit subsequent events
Client->>Stream: receive events
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 11 minutes and 27 seconds. Comment |
ricardomaraschini
changed the title
![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)
pkg/project/auth/watch_test.go (1)
278-308: Good test coverage for the happy path.The test correctly validates:
- Initial
Addedevents are emitted for existing namespaces- A
Bookmarkevent withInitialEventsAnnotationKeyannotation followsConsider adding edge case tests for completeness (optional):
- Empty namespace list: verify only the bookmark is emitted
- Single namespace: verify one Added event plus bookmark
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/project/auth/watch_test.go` around lines 278 - 308, Add two edge-case tests alongside TestSendInitialEventsBookmark: one that calls newTestWatcher with no namespaces to assert only a Bookmark event with the metav1.InitialEventsAnnotationKey annotation is emitted and no Added events are received, and another that calls newTestWatcher with a single namespace to assert exactly one Added event followed by the Bookmark; reuse the same test pattern (newTestWatcher(..., matchAllPredicate(), true, newNamespaces(...)), watcher.Watch(), and reading from watcher.ResultChan()) and include the same timeout checks and annotation assertions against project.Annotations[metav1.InitialEventsAnnotationKey].
🤖 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/project/auth/watch.go`:
- Around line 229-238: The bookmark event emitted when w.sendInitialEvents is
true lacks a ResourceVersion; update the watch.Bookmark event created in the
sendInitialEvents block (the emit call creating a watch.Event with
projectapi.Project) to set ObjectMeta.ResourceVersion to the latest known
resource version tracked by this watcher (from the knownProjects structure
initialized earlier). Locate the knownProjects store (referenced as
knownProjects) and use its latest ResourceVersion value when constructing the
projectapi.Project.ObjectMeta (e.g., set ObjectMeta.ResourceVersion = <latest
from knownProjects>) before calling w.emit so the bookmark complies with
WatchList/KEP-3157.
---
Nitpick comments:
In `@pkg/project/auth/watch_test.go`:
- Around line 278-308: Add two edge-case tests alongside
TestSendInitialEventsBookmark: one that calls newTestWatcher with no namespaces
to assert only a Bookmark event with the metav1.InitialEventsAnnotationKey
annotation is emitted and no Added events are received, and another that calls
newTestWatcher with a single namespace to assert exactly one Added event
followed by the Bookmark; reuse the same test pattern (newTestWatcher(...,
matchAllPredicate(), true, newNamespaces(...)), watcher.Watch(), and reading
from watcher.ResultChan()) and include the same timeout checks and annotation
assertions against project.Annotations[metav1.InitialEventsAnnotationKey].
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 324fb4c0-346b-4b3c-8817-33ca9b8db862
📒 Files selected for processing (3)
pkg/project/apiserver/registry/project/proxy/proxy.gopkg/project/auth/watch.gopkg/project/auth/watch_test.go
|
/retest |
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 |
support watch list by handling sendInitialEvents option, which sends initial state followed by a bookmark with initial-events-end annotation when enabled.
ricardomaraschini
force-pushed
the
OCPBUGS-78480
branch
from
08018d5 to
c9c3b8a
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ricardomaraschini The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/project/auth/watch_test.go (1)
26-47: Thread the bookmark flag through the test helper instead of mutating internal state.
newTestWatcheralways constructsNewUserProjectWatcher(..., false)and the new bookmark test flipswatcher.sendBookmarkafterward. That bypasses the constructor path used by production code and can hide regressions in the new plumbing.Proposed helper update
-func newTestWatcher(username string, groups []string, predicate storage.SelectionPredicate, includeAllExistingProjects bool, namespaces ...*corev1.Namespace) (*userProjectWatcher, *fakeAuthCache, chan struct{}) { +func newTestWatcher(username string, groups []string, predicate storage.SelectionPredicate, includeAllExistingProjects, sendBookmark bool, namespaces ...*corev1.Namespace) (*userProjectWatcher, *fakeAuthCache, chan struct{}) { @@ - return NewUserProjectWatcher(&user.DefaultInfo{Name: username, Groups: groups}, sets.NewString("*"), projectCache, fakeAuthCache, includeAllExistingProjects, predicate, false), fakeAuthCache, stopCh + return NewUserProjectWatcher(&user.DefaultInfo{Name: username, Groups: groups}, sets.NewString("*"), projectCache, fakeAuthCache, includeAllExistingProjects, predicate, sendBookmark), fakeAuthCache, stopCh }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/project/auth/watch_test.go` around lines 26 - 47, newTestWatcher currently forces the bookmark flag false and tests mutate watcher.sendBookmark directly; change newTestWatcher signature to accept a bookmark boolean (e.g., includeBookmark bool) and pass that value into NewUserProjectWatcher(..., includeBookmark) so tests exercise the constructor path; update callers/tests to pass true where the bookmark behavior is required and remove any direct mutations of watcher.sendBookmark; reference newTestWatcher and NewUserProjectWatcher to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/project/auth/watch_test.go`:
- Around line 26-47: newTestWatcher currently forces the bookmark flag false and
tests mutate watcher.sendBookmark directly; change newTestWatcher signature to
accept a bookmark boolean (e.g., includeBookmark bool) and pass that value into
NewUserProjectWatcher(..., includeBookmark) so tests exercise the constructor
path; update callers/tests to pass true where the bookmark behavior is required
and remove any direct mutations of watcher.sendBookmark; reference
newTestWatcher and NewUserProjectWatcher to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c19465da-674b-4f45-91d3-fc76e5b0119a
📒 Files selected for processing (3)
pkg/project/apiserver/registry/project/proxy/proxy.gopkg/project/auth/watch.gopkg/project/auth/watch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/project/auth/watch.go
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681,openshift/origin#31095 |
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 openshift/origin#31095 |
|
/retest |
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 openshift/origin#31095 |
|
@ricardomaraschini: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Support WatchList by handling
sendInitialEventsoption, which sends initial state followed by a bookmark withinitial-events-endannotation when enabled.Summary by CodeRabbit
New Features
Tests