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

fix: timediff typo #5462

Merged
merged 17 commits into from
Jan 6, 2022
Merged

fix: timediff typo #5462

merged 17 commits into from
Jan 6, 2022

Conversation

blackstrip
Copy link
Contributor

@blackstrip blackstrip commented Nov 28, 2021

Problem:

When i use the webui to make some timediff critera comparison in rules, it always return the TypeError: '>' not supported between instances of 'float' and 'str'

Check:

I tried so many value in critera form, it's just not work.
Then i found in operators.py, the _timediff is returned (utc_now - diff_target_utc).total_seconds() as a float number, the timedelta.total_seconds() is always return float type, but the critera-comparison is str or int

Fix:

Just convert the critera value to float, it will be fine.

@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Nov 28, 2021
@arm4b
Copy link
Member

arm4b commented Dec 3, 2021

@blackstrip Thanks for the contribution!

Could you please add the Changelog for this PR and, ideally some Unit test to cover the case.

Perhaps more details about how you caught this issue/error would help too for someone to review it.

@blackstrip
Copy link
Contributor Author

@blackstrip Thanks for the contribution!

Could you please add the Changelog for this PR and, ideally some Unit test to cover the case.

Perhaps more details about how you caught this issue/error would help too for someone to review it.

Hi @armab , Thanks for the reply!
The PR is updated.

@amanda11
Copy link
Contributor

amanda11 commented Dec 9, 2021

@blackstrip Thanks for contribution, the latest merge seems to have removed your CHANGELOG changes, could you update to master and try again with the changelog?

@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/XS PR that changes 0-9 lines. Quick fix/merge. labels Dec 10, 2021
@blackstrip
Copy link
Contributor Author

@amanda11 Thanks for review, updated.

Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

@blackstrip One small comment on changelog, and I wonder did you look to see if it was possible to write a UT for the fix as per @armab's suggestion?

CHANGELOG.rst Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Dec 10, 2021
Copy link
Contributor Author

@blackstrip blackstrip left a comment

Choose a reason for hiding this comment

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

Updated. please check again. thx.

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! 👍

@arm4b arm4b added the bug fix label Jan 5, 2022
@arm4b arm4b added this to the 3.7.0 milestone Jan 5, 2022
@arm4b arm4b added this to In progress in StackStorm v3.7.0 via automation Jan 5, 2022
@arm4b
Copy link
Member

arm4b commented Jan 5, 2022

@blackstrip Looks like the linting is failing https://github.com/StackStorm/st2/runs/4715712255?check_suite_focus=true#step:11:122
Can you please reformat the unit/test_operators.py with Black?

Additionally, new unit tests might need a bit more work per CI failures: https://github.com/StackStorm/st2/runs/4715712802

@blackstrip
Copy link
Contributor Author

@blackstrip Looks like the linting is failing https://github.com/StackStorm/st2/runs/4715712255?check_suite_focus=true#step:11:122 Can you please reformat the unit/test_operators.py with Black?

Additionally, new unit tests might need a bit more work per CI failures: https://github.com/StackStorm/st2/runs/4715712802

Hi @armab , thx for review. the case is udpated, but maybe git-action failed? please can you re-check?

@arm4b arm4b merged commit 9bfa96c into StackStorm:master Jan 6, 2022
StackStorm v3.7.0 automation moved this from In progress to Done Jan 6, 2022
@arm4b
Copy link
Member

arm4b commented Jan 6, 2022

That worked.
Merged, thanks!

@blackstrip blackstrip deleted the fix_timediff_typo branch April 20, 2022 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix size/M PR that changes 30-99 lines. Good size to review.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants