Skip to content
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

Extend mtbf time range to also support hours and minutes. #263

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

snarlistic
Copy link

@snarlistic snarlistic commented Mar 22, 2024

This PR changes the definition of "kube-monkey/mtbf".

Alike https://github.com/misgod-yy in #168 we needed a more fine grained control for the mean time between failure. A couple of years ago a coworker of mine took the misgod-yy patches and updated a now very old version of your software, fixed the problems with that code and we have been using this happily ever since for several years now.

The downside of this approach was that we were still using this very old version of the software and recently decided that's not okay anymore. Hence I ported our patches to the newest version of your code and improved it a tiny bit. Please bare with me, this is the first Golang programming I did.

Please review my work. I think it solves all the issues you had with the more early work of https://github.com/misgod-yy. It for instance sticks to minutes, hours and days, it removed the weird naming and last but not least is no breaking change as one can specify days as a simply integer (old style) and as an integer with an additional d-unit (new-style).

I hope you like it.

@snarlistic
Copy link
Author

snarlistic commented Mar 23, 2024

Please forgive me for the confusion, but it bothered me a bit that I had complicated matters a bit. I misunderstood certain aspects of the RandomTimeInRange code my coworker wrote at first and so I did not fully understand it already implements the ShouldScheduleChaos functionality. Hence I wrote some complicated code that in the end was not needed. This I realized today and for that reason I updated this pull request with the extra commit: 03a90f0. Hope you now even like it better. :)

@worldtiki
Copy link
Collaborator

Thanks for the contribution @snarlistic!
I'll take a closer look in the next few days :)

@lexfrei
Copy link

lexfrei commented Apr 17, 2024

@worldtiki any news?

…e issues and ugliness. Extensively documented the code to make it clear what happens as the algorithm is a bit of the complex kind.
@snarlistic
Copy link
Author

With a bit of shame I have to admit the code contained some issues and ugliness. My last commit solved both and I have this version running for some time now. Hopefully it is useful and you are open to accept my work.

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

Successfully merging this pull request may close these issues.

3 participants