-
Notifications
You must be signed in to change notification settings - Fork 315
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
configurable update strategy frequency and better update messaging #2320
Conversation
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
@@ -386,6 +386,8 @@ impl Service { | |||
pub fn update_package(&mut self, package: PackageInstall) { | |||
match Pkg::from_install(package) { | |||
Ok(pkg) => { | |||
outputln!(preamble self.service_group, |
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.
Excellent! It's very helpful to know what's happening and in which service group
@@ -109,8 +111,7 @@ impl ServiceUpdater { | |||
Err(TryRecvError::Empty) => return false, | |||
Err(TryRecvError::Disconnected) => {} | |||
} | |||
outputln!(preamble service.service_group, | |||
"Service Updater worker has died {}", "; restarting..."); | |||
debug!("Service Updater worker has died; restarting..."); |
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.
Much more appropriate
match val.parse::<i64>() { | ||
Ok(num) => num, | ||
Err(_) => { | ||
outputln!("Unable to parse {} from {} as a valid integer", |
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.
Might want to explain that we were trying to parse the update frequency and from which env var here so people know what was not a valid integer ;). Also mention you're falling back to X
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.
oh i actually did include the env var name, but I have the format args reversed. oops. fixing.
@mwrock just a few comments inline but this is a kick ass change. Will be super useful when we're demoing things. You've got a small formatting issue - looks like rustfmt might have missed a file and that's why you're failing travis. Merge it when you're ready |
Signed-off-by: Matt Wrock <matt@mattwrock.com>
@thesentinels approve |
🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests. |
Travis CI has started testing this PR. |
💖 Travis CI reports this PR passed. It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now. I just want you and the contributor to answer me one question: |
This fixes #2316 and also makes some small improvements to update output. The messages emitted when an update worker dies may raise a red flag in the mind of an observer when none is warranted. A dying update worker is a healthy milestone of the updater lifecycle. This is now a debug mrssage to protect those sensitive to death. This also clearly states the old and new release versions.
Signed-off-by: Matt Wrock matt@mattwrock.com