-
Notifications
You must be signed in to change notification settings - Fork 24
Solves issue #75 (i.e. squawk when ETA > .5) #76
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.
Good start!
server.js
Outdated
|
||
setInterval(function() { | ||
homu.retrieveSlaves(function(slaves) { | ||
for(var slaveName in slaves){ |
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.
Could we avoid using tabs here?
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.
Sorry! Must have left vim on paste mode. I'll fix that.
server.js
Outdated
setInterval(function() { | ||
homu.retrieveSlaves(function(slaves) { | ||
for(var slaveName in slaves){ | ||
if(slaves[slaveName].hasOwnProperty("runningBuilds")){ |
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.
Let's do this instead, and unindent the next lines:
if (!("runningBuilds" in slaves[slaveName])) {
continue;
}
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! I'll make these changes, too.
server.js
Outdated
if(slaves[slaveName].hasOwnProperty("runningBuilds")){ | ||
slaves[slaveName]["runningBuilds"].forEach(function(runningBuild){ | ||
if(runningBuild.eta > .5){ | ||
bot.say(config.channels[0], slaveName + " is past overdue! (ETA=" + runningBuild.eta + ")"); |
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.
We can make this output more useful by doing moment.unix(slaves[slaveName]["runningBuilds"]["times"][0]).fromNow()
instead of the raw ETA value, which will provide us with a more readable string: https://momentjs.com/docs/#/displaying/fromnow/
This will require adding moment.js to the package.json and installing it locally, too.
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! I'll look into adding moment and implementing it here.
Add moment for more comprehensive notification Change how slave iteration finds runningBuilds Remove tabs
@jdm Finished with requested changes |
Thanks! |
Add slave request in homu.js
Add appropriate interval w/ JSON iteration in server.js