API-1835: Scaffold event dispatcher #7543
Conversation
|
@p0lyn0mial: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. 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. |
WalkthroughAdds a new per-GVK input resource dispatcher for filtering and forwarding Kubernetes events, alongside comprehensive unit tests validating the dispatch logic across multiple filter scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
Comment |
|
/assign @benluddy @bertinatto @csrwng |
|
@p0lyn0mial: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. 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. |
|
|
||
| func newInputResourceDispatcher(filters map[schema.GroupVersionKind][]inputResourceEventFilter) *inputResourceDispatcher { | ||
| return &inputResourceDispatcher{ | ||
| eventsCh: make(chan event.GenericEvent, 10), |
There was a problem hiding this comment.
I think I will change it to an unbuffered channel.
(let's start simple)
1445b4c to
d2cb8af
Compare
|
/approve |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/openshiftmanager/input_resource_initializer.go`:
- Around line 47-49: The discoverInputResources function currently always
returns a "not implemented" error which blocks manager startup; change its
implementation in the inputResourceInitializer type so it returns an empty map
and nil error (e.g. make(map[string]*libraryinputresources.InputResources), nil)
to unblock startup, or implement the actual discovery logic used by
Start()/SetupWithManager if you prefer; locate and update discoverInputResources
(and ensure any callers like Start() handle an empty map) so the manager can
start successfully.
| func (r *inputResourceInitializer) discoverInputResources() (map[string]*libraryinputresources.InputResources, error) { | ||
| return nil, fmt.Errorf("not implemented") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find registration/usage of the initializer
rg -n -C3 '\bnewInputResourceInitializer\b|\binputResourceInitializer\b'Repository: openshift/hypershift
Length of output: 8472
🏁 Script executed:
# Get the file structure and locate the Start method
fd 'input_resource_initializer.go' --exec cat -n {}Repository: openshift/hypershift
Length of output: 5887
🏁 Script executed:
# Search for where this might be registered in the manager setup
rg -n -C5 'inputResourceInitializer|discoverInputResources'Repository: openshift/hypershift
Length of output: 10105
discoverInputResources blocks manager startup with "not implemented" error.
The initializer is registered in SetupWithManager and its Start() method immediately calls discoverInputResources(), which returns an error. This prevents the manager from starting. Either implement the discovery or return an empty map temporarily to unblock startup.
💡 Suggested interim fix
func (r *inputResourceInitializer) discoverInputResources() (map[string]*libraryinputresources.InputResources, error) {
- return nil, fmt.Errorf("not implemented")
+ // TODO: implement discovery.
+ return map[string]*libraryinputresources.InputResources{}, nil
}📝 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.
| func (r *inputResourceInitializer) discoverInputResources() (map[string]*libraryinputresources.InputResources, error) { | |
| return nil, fmt.Errorf("not implemented") | |
| } | |
| func (r *inputResourceInitializer) discoverInputResources() (map[string]*libraryinputresources.InputResources, error) { | |
| // TODO: implement discovery. | |
| return map[string]*libraryinputresources.InputResources{}, nil | |
| } |
🤖 Prompt for AI Agents
In
`@control-plane-operator/controllers/openshiftmanager/input_resource_initializer.go`
around lines 47 - 49, The discoverInputResources function currently always
returns a "not implemented" error which blocks manager startup; change its
implementation in the inputResourceInitializer type so it returns an empty map
and nil error (e.g. make(map[string]*libraryinputresources.InputResources), nil)
to unblock startup, or implement the actual discovery logic used by
Start()/SetupWithManager if you prefer; locate and update discoverInputResources
(and ensure any callers like Start() handle an empty map) so the manager can
start successfully.
| filters := d.filters[gvk] | ||
| if len(filters) == 0 { | ||
| d.eventsCh <- event.GenericEvent{Object: cObj} | ||
| return |
There was a problem hiding this comment.
Why dispatch the event if there are no filters? Is this an oversight?
There was a problem hiding this comment.
ok, so i misunderstood you question.
don't have a good answer here, i can change it to return if there a no filters and we can always adjust the code.
There was a problem hiding this comment.
it seems like, the initial list will contain "exact resources" only, xref: https://github.com/openshift/cluster-authentication-operator/blob/master/pkg/cmd/mom/input_resources_command.go#L18
so, it looks like we will always require some filters.
d2cb8af to
76bb8a5
Compare
|
@p0lyn0mial: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. 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. |
inputResourceDispatcher is a simple dispatcher that applies GVK scoped filters and forwards matching events. Each GVK has its own set of filters. Today these may include name/namespace checks, and in the future label selectors. Longer term, this dispatcher is expected to track which input resources are associated with which operator.
76bb8a5 to
c7512a9
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, p0lyn0mial 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 |
|
/verified by CI |
|
@p0lyn0mial: This PR has been marked as verified by 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. |
|
/lgtm |
|
/test e2e-aws |
|
@p0lyn0mial: 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. |
|
/hold Revision c7512a9 was retested 3 times: holding |
requires #7477