OCPBUGS-78480: add watchlist new semantic support to project watcher#661
OCPBUGS-78480: add watchlist new semantic support to project watcher#661dusk125 wants to merge 4 commits into
Conversation
support watch list by handling sendInitialEvents option, which sends initial state followed by a bookmark with initial-events-end annotation when enabled.
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 openshift/origin#31095 |
|
/jira refresh |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Bookmark support for project watch-list
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci-robot
added
jira/severity-critical
|
@dusk125: This pull request references Jira Issue OCPBUGS-78480, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@dusk125: This pull request references Jira Issue OCPBUGS-78480, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 openshift/origin#31095 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/project/auth/watch.go (1)
77-82:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHandle
authCache.Listerrors instead of discarding them.Line 78 drops the error from
authCache.List, and Line 80 can panic ifnamespacesis nil. Please return/propagate an error from constructor setup rather than proceeding with unknown state.Suggested direction
-func NewUserProjectWatcher(user user.Info, visibleNamespaces sets.String, projectCache *projectcache.ProjectCache, authCache WatchableCache, includeAllExistingProjects bool, predicate kstorage.SelectionPredicate, sendBookmark bool) *userProjectWatcher { - namespaces, _ := authCache.List(user, labels.Everything()) +func NewUserProjectWatcher(user user.Info, visibleNamespaces sets.String, projectCache *projectcache.ProjectCache, authCache WatchableCache, includeAllExistingProjects bool, predicate kstorage.SelectionPredicate, sendBookmark bool) (*userProjectWatcher, error) { + namespaces, err := authCache.List(user, labels.Everything()) + if err != nil { + return nil, err + } + if namespaces == nil { + return nil, errors.New("auth cache returned nil namespace list") + } @@ - return w + return w, nil }As per coding guidelines, "
**/*.go: Go security (prodsec-skills) - Never ignore error returns".🤖 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/project/auth/watch.go` around lines 77 - 82, In the NewUserProjectWatcher constructor function, the error returned by authCache.List on line 78 is being ignored with a blank identifier, and line 80 can panic if namespaces is nil. Instead of discarding the error, you must check the return value from authCache.List and propagate any error to the caller. This requires updating the function signature to return both *userProjectWatcher and an error (changing from returning only *userProjectWatcher), then checking if the error from authCache.List is non-nil and returning it early if so, rather than proceeding with potentially nil or invalid state.Source: Coding guidelines
🤖 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 `@pkg/project/auth/watch.go`:
- Around line 229-236: The bookmark event emitted in the w.emit() call at this
location includes only the InitialEventsAnnotationKey annotation in the
ObjectMeta but is missing the ResourceVersion field. Update the
projectapi.Project object's ObjectMeta to include both the ResourceVersion and
the InitialEventsAnnotationKey annotation to match upstream watch-list semantics
as documented in KEP-3157. The ResourceVersion should reflect the current watch
state at the point the bookmark is being emitted.
---
Outside diff comments:
In `@pkg/project/auth/watch.go`:
- Around line 77-82: In the NewUserProjectWatcher constructor function, the
error returned by authCache.List on line 78 is being ignored with a blank
identifier, and line 80 can panic if namespaces is nil. Instead of discarding
the error, you must check the return value from authCache.List and propagate any
error to the caller. This requires updating the function signature to return
both *userProjectWatcher and an error (changing from returning only
*userProjectWatcher), then checking if the error from authCache.List is non-nil
and returning it early if so, rather than proceeding with potentially nil or
invalid state.
🪄 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: Enterprise
Run ID: 0e0a97fc-bfa0-4801-b272-0789eef3b06c
📒 Files selected for processing (3)
pkg/project/apiserver/registry/project/proxy/proxy.gopkg/project/auth/watch.gopkg/project/auth/watch_test.go
|
/testwith ? |
|
/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 |
1 similar comment
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 openshift/origin#31095 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 openshift/origin#31095 |
|
@dusk125: 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. |
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 openshift/origin#31313 |
| Object: &projectapi.Project{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Annotations: map[string]string{metav1.InitialEventsAnnotationKey: "true"}, | ||
| ResourceVersion: w.bookmarkResourceVersion, |
There was a problem hiding this comment.
Have you implemented a "resume" watching here ? My understanding is that the ResourceVersion is completely ignored (except when it is "zero"). Can you elaborate a little over this one ?
There was a problem hiding this comment.
This was a Claude-ism, It was concerned that having a blank ResourceVersion would break RetryWatchers, so it tried to give a "meaningful" value here.
I've reverted this for now and will run the updated origin tests to see if it was necessary or not.
|
/testwith openshift/openshift-apiserver/main/e2e-aws-ovn openshift/cluster-openshift-apiserver-operator#681 openshift/origin#31313 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
support watch list by handling sendInitialEvents option, which sends initial state followed by a bookmark with initial-events-end annotation when enabled.
Summary by CodeRabbit
New Features
Bug Fixes