-
Notifications
You must be signed in to change notification settings - Fork 302
Conversation
Note: didn't add prometheus (and friends) in gopath. |
@miekg before I try to review this in detail: on a quick glance, it looks like all the later commits just improve upon the original one, rather than making some distinct changes? In that case, they should probably all be squashed into a single commit. You mentioned that you haven't added the prometheus dependency, but you didn't explain why? One detail I noticed is that the copyright years are wrong. It should clearly be 2016 for the newly added file; and as far as I can see, the convention around here is to also bump the year of existing files when making substantial changes. (Strictly speaking, when making changes, the original date should be kept along with the updated one; and also, since I guess you haven't signed over copyright to CoreOS Inc., they should mention your or your employer's name... But as far as I can see, these rules haven't been followed for fleet in the past either -- so not sure how to handle this...) BTW, would it be feasible to add any unit tests and/or functional tests for this new feature?... |
@miekg I know your situation changed recently - let us know if you have On Tue, Mar 15, 2016 at 6:07 PM Olaf Buddenhagen notifications@github.com
|
[ Quoting notifications@github.com in "Re: [fleet] prometheus metrics for ..." ]
Thanks. I don't think I can spend cycles on this one... Thanks for stepping in. /Miek Miek Gieben |
Superseded closed by #1524 |
This PR add prometheus metrics to fleet.