feat: validate crm high and low threshold input #4142
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: JaviEst <javier.esteban_99@hotmail.com>
JaviEst
force-pushed
the
validate-crm-input
branch
from
7783868 to
d8aa405
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR adds validation for CRM (Critical Resource Monitoring) high and low threshold configuration values to address issue #2931. The changes ensure that percentage thresholds are between 0 and 100, and that low thresholds are less than high thresholds.
Key changes:
- Added
get_crm_config()helper method to theCrmclass to retrieve CRM configuration - Implemented validation logic in
low()andhigh()functions to check percentage bounds and threshold ordering - Added comprehensive test cases covering edge cases: values over 100, low >= high, high <= low, and valid configurations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| crm/main.py | Added validation logic to low() and high() threshold configuration functions, and refactored config retrieval into a helper method |
| tests/crm_test.py | Added four new test methods to verify threshold validation for percentage bounds and ordering constraints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: JaviEst <javier.esteban_99@hotmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: JaviEst <javier.esteban_99@hotmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: JaviEst <javier.esteban_99@hotmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hoping I can get some reviews please @qiluo-msft @mssonicbld @yxieca |
|
|
||
| crm_info = ctx.obj["crm"].get_crm_config() | ||
| threshold_type = crm_info.get(type_attr, 'percentage') | ||
| if threshold_type == 'percentage': |
There was a problem hiding this comment.
this type logic wasn't there in the original version. The yang model looks for crm_threshold_type looks like this:
typedef crm_threshold_type {
type string {
length 1..64;
pattern "percentage|used|free|PERCENTAGE|USED|FREE";
}
}
It looks like your change restricts the type check to only be done when the value is percentage, which seems like the right thing to do. But what should we be doing in the used/free case? are those not also percents? maybe we should be doing this check regardless of type?
|
|
||
| crm_info = ctx.obj["crm"].get_crm_config() | ||
| threshold_type = crm_info.get(type_attr, 'percentage') | ||
| if threshold_type == 'percentage': |
There was a problem hiding this comment.
you need to accept PERCENTAGE as well, as per the yang-model
| 'Low threshold value must be between 0 and 100 for ' | ||
| 'percentage type.') | ||
|
|
||
| high_value = crm_info.get(high_attr, None) |
There was a problem hiding this comment.
if you're checking the type for the low value, shouldn't you check it for the high value as well? or is it assumed that if the low value is a percentage, the high value must be as well? if so, is that assumption captured in the yang-model somewhere?
| f"{low_value} for percentage type." in result.output | ||
| ) | ||
|
|
||
| def test_crm_config_thresholds_valid_values(self): |
There was a problem hiding this comment.
either here or in another test case, you should check the corner cases, where thresholds are 0/100, and when they are equal (so for low/high threshold, you could check 0/100, 0/0, 100/100)
3 participants
What I did
Resolved issue #2931 by validating the high and low threshold input values.
How I did it
Updated the high and low threshold config functions to validate both thresholds are between 0 and 100 for percentage type as well as validate the low threshold if smaller than the high threshold.
How to verify it
Run new implemented tests to verify the validation logic is enforced.