Skip to content
This repository has been archived by the owner on Jan 11, 2021. It is now read-only.

Add monitoring of container image deployments #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nathancashmore
Copy link

No description provided.

Copy link
Member

@whs whs left a comment

Choose a reason for hiding this comment

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

Thanks for your pull requests. I've added some comments. If you need help on refactoring, please feel free to ask.

@@ -1,4 +1,5 @@
module.exports = [
require('./waitingpods'),
require('./longnotready'),
require('./newdeployment'),
Copy link
Member

Choose a reason for hiding this comment

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

We're using one tab indentation. Please reindent this line and the monitor file.


class DeploymentStatus extends EventEmitter{
constructor(){
super();
Copy link
Member

Choose a reason for hiding this comment

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

If you have nothing but super(); in your constructor, it can be omitted.

Copy link
Member

Choose a reason for hiding this comment

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

Actually from below, we'll need the constructor to initialize the state store.

}

async check(){
let containers = await kube.getContainerStatuses();
Copy link
Member

Choose a reason for hiding this comment

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

I think rather than checking for newly deployed containers, we should check for newly deployed pods. This is available in kube.getPods();

_key: `${item.pod.metadata.name}/${item.name}`,
});

global.image = item.image;
Copy link
Member

Choose a reason for hiding this comment

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

If this is stateful then the state should be stored as instance attribute, rather than as a global variable.

@phillipj
Copy link
Contributor

@nathancashmore would you mind if I hi-jacked this PR and fixed the raised review issues? I'd really appreciate having the bot notify us about new deployments!

@nathancashmore
Copy link
Author

@phillipj go for it.

@phillipj
Copy link
Contributor

Alrighty, thanks. I'll for sure credit you as the author in the commit!

@phillipj
Copy link
Contributor

phillipj commented Jul 4, 2018

Updates after having done some initial testing of this; we decided to send a Slack message from our deployment script with a cURL command instead. Looks something like this:

screen shot 2018-07-04 at 15 35 54

That in combination with the already existing monitors that will tell us about containers having trouble, covers our needs.

Bottom line, I was a little quick saying I'll hijack this PR with the goals of getting it merged. Sorry for those who waited in vain.

@AshMartian
Copy link
Contributor

This may be a little unnecessary for some, this should be conditional with a configuration flag.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants