- 
                Notifications
    You must be signed in to change notification settings 
- Fork 12
Consolidate Status and Condition reporting #265
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, the possibility to call the Report method on the EventStatusRecorder makes it well readable, also the Conditions() method in the api is very clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much like the usage of an interface and removing all this duplicate code. I have 2 improvement ideas, see comments.
adjust string separator add copyright header modified error return for prefixclaimcontroller implement feedback - logging message location only perform update call if the condition changed
6e8e185    to
    ea07072      
    Compare
  
    06ac2a7    to
    a454dde      
    Compare
  
    * refactor event/status reporting * only perform update call if the condition changed * don't use private fields of EventStatusRecorder * set ready false at beginning of reconcile loop * add information about kind image loading with containerd * fix match string for chainsaw tests * change format string to wrap error
This PR shows one way in which we can consolidate the reporting of status and conditions into a common function.
It exposes access to the EventRecorder so events can be emitted independently of condition updates, and also logs changes in conditions as well as errors.
Would be happy to receive feedback and take suggestions as to how we can improve this functionality.