-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: add built-in interval entity #1179
Conversation
@kkedziak-splunk some UI tests are failing, please take a look |
@artemrys it was a typo, I fixed it on Friday |
Please add a description with an explanation of what was done in this PR, how are existing customers impacted. Do we migrate existing TAs to this new interval field? |
{ | ||
"type": "regex", | ||
"errorMsg": f"{self['label']} must be either a non-negative number or -1.", | ||
"pattern": r"^(?:-1|0(?:\.\d+)?|[1-9]\d*(?:\.\d+)?)$", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex doesn't work with a value like 00123
. Users ideally don't put values like this, but it would be good if we can handle it.
Also, should we have any maximum value? The modinput should at least run once a year or once in six months, something like that?
We can use regex and range validators for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no limits for intervals in Splunk, I don't think we should come up with some default value.
There are min
and max
fields in the options so developers can set it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add leading 0's to the regex.
tests/unit/test_schema.py
Outdated
"-10", | ||
"-1.0", | ||
"-0.1", | ||
"01", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should 01
be a valid value?
Please use the new PR template, you can find it in .github folder. Can you please expand on the user experience section? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please run pytest tests/unit --cov splunk_add_on_ucc_framework --cov-report html; open htmlcov/index.html
and make sure you have test coverage for your new code?
We need to do it like this before we have a tool to do it for us.
# Conflicts: # splunk_add_on_ucc_framework/commands/build.py # tests/smoke/test_ucc_build.py
"$ref": "#/definitions/IntervalValue" | ||
}, | ||
"options": { | ||
"anyOf": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that anyOf seems unnecessary , as you passed only one possibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I forgot to remove it.
Ignore as it should work similarly to logginTab change, so UI code is not affected eventually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just a small comment which can land in a separate small PR.
} | ||
|
||
for name in ("help", "required", "defaultValue", "tooltip"): | ||
if name in definition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a test that covers this line?
Issue number: ADDON-68645
Summary
Changes
This PR adds a new entity type:
interval
.It is a text field used for providing numeric value for intervals, i.e. -1, 0 or a positive number (integer or float).
Regex:
^(?:-1|\d+(?:\.\d+)?)$
The old intervals added as text fields are migrated to the new solution. There may be a difference in the regex used to check the value - the popular solution allowed to input all negative numbers, while it should not be allowed.
User experience
Although the old interval will be migrated automatically during the build step, the user experience should be the same. The UI will not change. However, the regex validation is a bit different - currently some TA's allow specifying incorrect interval values.
Checklist
If your change doesn't seem to apply, please leave them unchecked.