-
Notifications
You must be signed in to change notification settings - Fork 67
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
Redesign restart events #152
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Katherine Stanley <[email protected]>
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.
Thanks for the proposal. I left some nits, but I'm +1 overall.
101-redesign-restart-events.md
Outdated
This will be a change for users, however since the events being emitted aren't versioned there is no clear way to indicate this change. | ||
However, we can introduce this change at the same time as the v1 API. | ||
When the v1 API lands users are likely to be reviewing the changelog more thoroughly than for other releases, increasing the likelihood that they will not be caught out by the change. |
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'm fine with this. But you can also consider doing it in 0.46 which has the CHANGELOG full of similar changes.
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've updated it to say we should aim for 0.46 release, but if we miss that use the release where the v1 API lands
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.
LGTM. But should we consider having such events for other pods like bridge, cruise control and entity operator?
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.
Are we also updating or adding log messages?
In #10958 we mention the possibility to remove events with the benefit of simplified maintenance. I'm ok to keep them, but maybe we should add the removal as rejected alternative with some motivation.
Signed-off-by: Katherine Stanley <[email protected]>
@ppatierno I think we could, however I think that is outside the scope of this proposal and related issue.
@fvaleri the proposal includes updating the restart reason to include the pod name, however there are no other log message changes included. The issue was around the events being hard to find, not the operator logs being unclear. |
Signed-off-by: Katherine Stanley <[email protected]>
Signed-off-by: Katherine Stanley <[email protected]>
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.
LGTM. Thanks for addressing my comments.
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.
LGTM :)
This proposal has now 4 +1 (binding) votes and 2 +1 (non binding). Waiting one more day for last minute additional feedback, then @katheris I think you can merge it. |
No description provided.