-
-
Notifications
You must be signed in to change notification settings - Fork 782
RFR: Timer should not be sensor #902
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
Conversation
|
Nice - going forward, under which service / where the timer will run? |
I am thinking RulesEngine. Thoughts? |
|
@lakshmi-kannan Honestly not sure aka haven't decided yet what's the best place for 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 have no idea what this test was trying to achieve. I removed this test. Let me know if anyone has objections.
|
@Kami For now, I put the timer in rules_engine. We can change this later if needed. |
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.
Sweet, we can finally get rid of this "hack" - we just need to make sure we never put any sensors in the core pack otherwise it's going to break (we won't detect those sensors).
|
After a quick pass it looks good, but we also need to add test cases for it. And I also want to test it before merging to make sure we aren't introducing any regressions. |
Do you mean the timer? I am planning to add some. Was thinking another PR. Let me get started on that. |
|
@lakshmi-kannan Yep and ideally we would do it in this PR :) |
|
@Kami https://github.com/StackStorm/st2/pull/921/files (opened a PR against this PR inception style) |
|
Let's see if I can resolve the merge conflict and get this merged today. Challenge accepted. |
What?
Timer is not a sensor anymore. It would now be part of st2 process (rules_engine for now but we can move if needed)
Things fixed
TODO