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

Propagate errors of GenevaLogging controller #3221

Merged
merged 4 commits into from
Nov 8, 2023
Merged

Propagate errors of GenevaLogging controller #3221

merged 4 commits into from
Nov 8, 2023

Conversation

mbarnes
Copy link
Contributor

@mbarnes mbarnes commented Oct 18, 2023

Which issue this PR addresses:

[ARO-2994] Propagate errors of GenevaLogging controller
under epic [ARO-1627] Propagate Errors and Statuses of ARO Controllers to ARO Cluster Operator

What this PR does / why we need it:

Adds a GenevaLoggingReady "check" condition to the cluster.aro.openshift.io/v1alpha1 singleton object that exposes controller errors.

Test plan for issue:

Manual testing using a custom operator image.

Is there any documentation that needs to be updated for this PR?

None that I'm aware of.

SudoBrendan
SudoBrendan requested changes Oct 23, 2023
View reviewed changes
Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

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

This code looks good, but I think we should follow our standard pattern for propoating the controller status conditions by using base.AROController; it'll save us some duplicate code and make things easier to maintain over time.

pkg/operator/controllers/genevalogging/genevalogging_controller.go Outdated Show resolved Hide resolved
@mbarnes
Copy link
Contributor Author

mbarnes commented Oct 31, 2023

I rewrote my changes from scratch since AROController handles conditions quite a bit differently than my initial attempt. Hardest part of ARO-2994 was understanding what was being asked: the breadcrumb there was misleading, but I gleaned the pattern from the other controllers.

@tsatam
Copy link
Collaborator

tsatam commented Oct 31, 2023

I rewrote my changes from scratch since AROController handles conditions quite a bit differently than my initial attempt. Hardest part of ARO-2994 was understanding what was being asked: the breadcrumb there was misleading, but I gleaned the pattern from the other controllers.

The PR linked in the breadcrumb is admittedly not the most up-to-date example of how to do this - that initial PR predates the existence of the base controller and helper functions. Perhaps we should update any future stories to point to a more recent example, such as this PR itself?

tsatam
tsatam requested changes Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@tsatam tsatam left a comment

Choose a reason for hiding this comment

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

Changes to the controller LGTM, I like the refactor to split the "work" into a simple function and let Reconcile just focus on controller things.

I think we should capture our expected conditions in the tests, similar to other PRs such as #2965.

pkg/operator/controllers/genevalogging/genevalogging_test.go Show resolved Hide resolved
@mbarnes
Copy link
Contributor Author

mbarnes commented Nov 1, 2023

Added condition checks to the unit tests, although there's currently no test cases that cause an error.

@mbarnes mbarnes requested a review from tsatam November 2, 2023 11:33
tsatam
tsatam approved these changes Nov 2, 2023
View reviewed changes
Copy link
Collaborator

@tsatam tsatam left a comment

Choose a reason for hiding this comment

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

Tests LGTM. The lack of test coverage over error scenarios is concerning, but that can be a follow-up effort.

@SudoBrendan SudoBrendan merged commit 67d06bd into Azure:master Nov 8, 2023
18 checks passed
@mbarnes mbarnes mentioned this pull request Nov 9, 2023
Open
ventifus pushed a commit to ventifus/ARO-RP that referenced this pull request Feb 7, 2024
@mbarnes @ventifus
Propagate errors of GenevaLogging controller (Azure#3221)
561e3e2 
* genevalogging: Use AROController as base type
* genevalogging: Split off business logic for uniform error handling
* genevalogging: Add condition for controller status
* genevalogging: Check status conditions in unit tests

---------

Co-authored-by: Matthew Barnes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsatam tsatam tsatam approved these changes

@SudoBrendan SudoBrendan SudoBrendan approved these changes

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@bennerv bennerv Awaiting requested review from bennerv bennerv is a code owner

@hawkowl hawkowl Awaiting requested review from hawkowl hawkowl is a code owner

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@jharrington22 jharrington22 Awaiting requested review from jharrington22 jharrington22 is a code owner

@cblecker cblecker Awaiting requested review from cblecker cblecker is a code owner

@facchettos facchettos Awaiting requested review from facchettos

@cadenmarchese cadenmarchese Awaiting requested review from cadenmarchese cadenmarchese is a code owner

@UlrichSchlueter UlrichSchlueter Awaiting requested review from UlrichSchlueter UlrichSchlueter is a code owner

@s-amann s-amann Awaiting requested review from s-amann

@Shivkumar13 Shivkumar13 Awaiting requested review from Shivkumar13

@yjst2012 yjst2012 Awaiting requested review from yjst2012 yjst2012 is a code owner

@anshulvermapatel anshulvermapatel Awaiting requested review from anshulvermapatel anshulvermapatel is a code owner

@hlipsig hlipsig Awaiting requested review from hlipsig hlipsig is a code owner

Assignees
No one assigned
Labels
loki ready-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mbarnes @tsatam @SudoBrendan