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

Feature/equal to #102

Merged
merged 4 commits into from
Mar 13, 2020
Merged

Conversation

derekgroh
Copy link
Contributor

Pull Request Checklist

#101

General

  • Update Changelog following the conventions laid out on Our CHANGELOG Guidelines

  • Update README with any necessary configuration snippets

  • Binstubs are created if needed

  • RuboCop passes

  • Existing tests pass

Purpose

Match the operator logic around cpu and memory checks as found in their Linux counterparts. Use greater than or equal to instead of greater than.

Known Compatibility Issues

None

@derekgroh derekgroh requested a review from a team October 30, 2019 18:44
Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

LGTM

Needs changelog section header.

@@ -5,7 +5,7 @@ This CHANGELOG follows the format listed at [Our CHANGELOG Guidelines ](https://
Which is based on [Keep A Changelog](http://keepachangelog.com/)

## [Unreleased]

- update cpu and memory checks to follow comparison operator in linux plugins check (greater than or equal to) (@derekgroh)
Copy link
Member

Choose a reason for hiding this comment

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

It's important to note that this change while unlikely will negatively affect someone some people may be very specific and this breaks their old expectations.

Suggested change
- update cpu and memory checks to follow comparison operator in linux plugins check (greater than or equal to) (@derekgroh)
### [Breaking Changes]
- update cpu and memory checks to follow comparison operator in linux plugins check (greater than or equal to) (@derekgroh)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A noteworthy change, but this is not breaking by definition.

Copy link
Member

@majormoses majormoses Oct 31, 2019

Choose a reason for hiding this comment

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

It depends on how you define breaking. It absolutely changes the expectation of the outcome what will happen given the same input. Even though its a good call it is absolutely breaking. It does not matter if it affects 1 or millions of users, breaking is breaking. It's the only way semver can be trusted. When in doubt version major, it will save you later when someone complains about you breaking their particular edgecase you never thought about.

Take for example:

# use the same value as input
current_value = 50

# testing old behavior would result in an OK
if current_value > 50
  critical
else
  ok
end

# testing new behavior will result in a CRITICAL
if current_value >= 50
  critical
else
  ok
end

@derekgroh derekgroh merged commit 89a5d7f into sensu-plugins:master Mar 13, 2020
derekgroh added a commit to derekgroh/sensu-plugins-windows that referenced this pull request Mar 13, 2020
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.

2 participants