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.

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

Add Topic Operator refactoring: handlers #10412

Merged
merged 7 commits into from
Sep 5, 2024
Merged

Conversation

fvaleri
Copy link
Contributor

@fvaleri fvaleri commented Aug 2, 2024

Type of change

  • Refactoring

Description

This change completes the TO refactoring by introducing a set of handlers and related tests. Each handler contains the logic used to call an external system, and helps to keep the BatchTopicController focused on the reconciliation logic. This also introduces the Results object, used toeliminate all side effects from reconciliation functions.

The fix for #9972 is also included.
All unit, integration and relevant system tests have been run locally.

Checklist

  • Write tests
  • Make sure all tests pass
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging

@fvaleri fvaleri requested review from tombentley and ppatierno August 2, 2024 12:28
@fvaleri fvaleri added this to the 0.43.0 milestone Aug 2, 2024
@ppatierno
Copy link
Member

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj scholzj modified the milestones: 0.43.0, 0.44.0 Aug 19, 2024
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Thanks @fvaleri!

@fvaleri fvaleri requested a review from tombentley August 21, 2024 14:00
@fvaleri fvaleri force-pushed the to-ref-btc branch 2 times, most recently from 757ae49 to 3b07dc3 Compare August 23, 2024 10:31
@fvaleri
Copy link
Contributor Author

fvaleri commented Aug 23, 2024

@tombentley hopefully, I addressed all your comments, including the ascii art one :)

Failed test is unrelated.

@fvaleri
Copy link
Contributor Author

fvaleri commented Aug 28, 2024

/packit test --labels regression

@fvaleri fvaleri force-pushed the to-ref-btc branch 3 times, most recently from b19b588 to 2a595be Compare September 2, 2024 15:56
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @fvaleri

This change completes the TO refactoring by introducing a set of handlers and related tests.
Each handler contains the logic used to call an external system, and helps to keep the BatchTopicController focused on the reconciliation logic.
This also introduces the Results object, used toeliminate all side effects from reconciliation functions.

The fix for strimzi#9972 is also included.
All unit, integration and relevant system tests have been run locally.

Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
@fvaleri
Copy link
Contributor Author

fvaleri commented Sep 3, 2024

/packit test --labels regression

@fvaleri fvaleri removed the request for review from ppatierno September 4, 2024 06:27
@fvaleri fvaleri added the ready for merge Label for PRs which are ready for merge label Sep 5, 2024
@scholzj
Copy link
Member

scholzj commented Sep 5, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Sep 5, 2024

/azp run build

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj scholzj merged commit f430670 into strimzi:main Sep 5, 2024
27 checks passed
@fvaleri fvaleri deleted the to-ref-btc branch September 5, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for merge Label for PRs which are ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants