Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

improve: remove EventSourceInitializer #2257

Merged
merged 8 commits into from
Mar 12, 2024
Merged

improve: remove EventSourceInitializer #2257

merged 8 commits into from
Mar 12, 2024

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Mar 1, 2024

Move EventSourceIntiailizer directly to Reconciler. Based on the patter, if properly done, there will be always EventSource. Still can return empty map.

@csviri csviri linked an issue Mar 1, 2024 that may be closed by this pull request
Consider moving the prepareEventSources directly to Reconciler in v5 #2029
Closed
@csviri
Copy link
Collaborator Author

csviri commented Mar 1, 2024

We should consider (I'm leaning to this) not even provide a default implementation, although might be confusing for someone who starts first, that is that part not simple to understand first (like how Informers works and why essential), but at least moves users news to operators to good direction.

what do you tink @metacosm @jcechace ?

metacosm
metacosm reviewed Mar 1, 2024
View reviewed changes
...ator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Reconciler.java Outdated
* @param eventSources to name
* @return even source with default names
*/
static Map<String, EventSource> nameEventSources(EventSource... eventSources) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that we move all these methods to a different utilty class. They have nothing to do with Reconciler itself and pollute its API, imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will move it for now, was thinking to also have have EventSources "Named" like now with the Dependent Resources. Will make a separate PR. That would make these methods basically obsolete.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added EventSourceUtils.

metacosm
metacosm reviewed Mar 1, 2024
View reviewed changes
...work-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/EventSource.java Outdated
* not changed). To register EventSources with so that your reconciler is triggered, please make
* your reconciler implement
* {@link io.javaoperatorsdk.operator.api.reconciler.EventSourceInitializer}.
* not changed). To register EventSources with so that your reconciler is triggered.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed.

@metacosm
Copy link
Collaborator

metacosm commented Mar 1, 2024

We should consider (I'm leaning to this) not even provide a default implementation, although might be confusing for someone who starts first, that is that part not simple to understand first (like how Informers works and why essential), but at least moves users news to operators to good direction.

what do you tink @metacosm @jcechace ?

The default implementation should stay because, imo, most people will never need to implement this method. What would the implementation be for someone who only uses managed workflows, for example?

@csviri
Copy link
Collaborator Author

csviri commented Mar 1, 2024 •
edited
Loading

The default implementation should stay because, imo, most people will never need to implement this method. What would the implementation be for someone who only uses managed workflows, for example?

ok that is good argument, to keep the default impl. (Although most is probably a strong word, since that is the case only for simple controllers)

@csviri csviri requested a review from metacosm March 4, 2024 09:02
@csviri csviri force-pushed the remove-event-source-initializer branch from cdc399c to 1ea7b72 Compare March 6, 2024 08:12
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2024
@metacosm metacosm force-pushed the remove-event-source-initializer branch from c076f9f to 75378b3 Compare March 11, 2024 17:19
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2024
@metacosm metacosm force-pushed the remove-event-source-initializer branch from 75378b3 to c076f9f Compare March 11, 2024 17:43
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2024
@csviri csviri force-pushed the remove-event-source-initializer branch from c076f9f to 43f3002 Compare March 11, 2024 17:53
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2024
metacosm
metacosm requested changes Mar 11, 2024
View reviewed changes
Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation needs updates.

docs/documentation/dependent-resources.md Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2024
csviri and others added 8 commits March 12, 2024 14:13
@csviri
improve: remove EventSourceInitializer
b899904 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
@csviri
fix docs
c0b61fb 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
@csviri
introduce event source utils
d59d145 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
@csviri
docs update
6909189 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
@metacosm @csviri
docs: improve wording
1f3387d 
Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm @csviri
fix: actually remove EventSourceInitializer
e0888b8 
Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm @csviri
fix(docs): update standalone example documentation
7c3fb27 
Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm @csviri
refactor: improve wording
8c846cf 
Signed-off-by: Chris Laprun <claprun@redhat.com>
@csviri csviri force-pushed the remove-event-source-initializer branch from 1a78328 to 8c846cf Compare March 12, 2024 13:15
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2024
@csviri csviri merged commit 106f14b into next Mar 12, 2024
20 checks passed
@csviri csviri deleted the remove-event-source-initializer branch March 12, 2024 13:43
@metacosm metacosm mentioned this pull request Mar 21, 2024
Closed
csviri added a commit that referenced this pull request Apr 10, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
5d63020 

Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
metacosm added a commit that referenced this pull request Apr 10, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
0dc1fd0 

Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
metacosm added a commit that referenced this pull request Apr 11, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
4875c08 

Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
csviri added a commit that referenced this pull request May 17, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
748fb4e 

Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
csviri added a commit that referenced this pull request May 17, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
d09f194 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request May 21, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
cee25b2 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request Jun 17, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
ab06720 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
csviri added a commit that referenced this pull request Jun 17, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
2b25e7d 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
metacosm added a commit that referenced this pull request Jul 8, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
9b3e99c 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
metacosm added a commit that referenced this pull request Jul 9, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
1d51b66 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
metacosm added a commit that referenced this pull request Jul 12, 2024
@csviri @metacosm
improve: remove EventSourceInitializer (#2257)
136b384 
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Co-authored-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Attila Mészáros <csviri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metacosm metacosm metacosm approved these changes

@adam-sandor adam-sandor Awaiting requested review from adam-sandor

@andreaTP andreaTP Awaiting requested review from andreaTP

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider moving the prepareEventSources directly to Reconciler in v5
3 participants
@csviri @metacosm @openshift-merge-robot