Hi Scot,

I reviewed both PRs.

They both look fine to me, but I think they are missing a few things. Reviewers are always asking for something... sorry, but that's part of the normal process!

Let me explain how I reviewed them. Both PRs could have been handled the same way, but I intentionally treated them differently:

For PR #66, I asked you to add something before approval. Once approved, you will merge it yourself.

For PR #65, I approved and merged it. Normally, I would let the author merge their own PR, but this time I merged it and opened a new PR to complete it, so you can see what I would like to add.

I think the first approach should be the normal case:

• Someone brings a feature, and the complete feature remains attributed to them. The author stays the author, and the reviewer stays the reviewer.

• With the second approach, merging your PR first lets us test it directly, but opening another PR to complete it swaps the roles: I become the author and you become the reviewer.

Both approaches have pros and cons, and I have no problem with either.

In both cases, I may also be wrong, so don't hesitate to correct me. And don't worry too much about introducing a bug from time to time — that's also part of the development lifecycle.

For now, the repository does not enforce a two-person review process. I think we should enforce it once we feel the process is ready. Let me know if you think it would be better to enforce it now.

We can already follow the rule without technically enforcing it. I still merge some PRs I create myself, but I can understand that this makes the process harder to follow and less clear for everyone.

This may all be trivial for you, so sorry for the long explanation if that's the case!

And thanks for getting involved!

Bruno

Yesterday I closed the two pull requests I opened for the wrong xymon.spec file, and created two new pull requests for the xymon.spec file in the xymon-rpm repo.  In short, these are the two issues:

 

• On the client rpm install /etc/xymon-client/clientlaunch.d is not created.  It should be, Debian already creates it.
• On the server rpm install, it places the httpd config for xymon but doesn’t signal httpd to reload configs.  That scenario is a little less clear-cut as to whether it should be automatic.  My opinion is that it should be. 

 

 

I opened pull requests for both changes.  All tests have passed and they’re waiting for review.

 

Scot Kreienkamp | Principal Applications Infrastructure Architect | La-Z-Boy Incorporated 
(734) 384-6403 | 1-734-915-1444 | Scot.Kreienkamp@la-z-boy.com
One La-Z-Boy Drive | Monroe, Michigan 48162 | la-z-boy.com
facebook.com/lazboy  | instagram.com/lazboy | youtube.com/lazboy

This message is intended only for the individual or entity to which it is addressed. It may contain privileged, confidential information which is exempt from disclosure under applicable laws. If you are not the intended recipient, you are strictly prohibited from disseminating or distributing this information (other than to the intended recipient) or copying this information. If you have received this communication in error, please notify us immediately by e-mail or by telephone at the above number. Thank you.

_______________________________________________
Xymon mailing list -- xymon@xymon.com
To unsubscribe send an email to xymon-leave@xymon.com