Add support for live rule field in sift-client #463
Conversation
| asset_tag_ids: list[str] | None = None | ||
| contextual_channels: list[str] | None = None | ||
| is_external: bool = False | ||
| is_live_evaluation_enabled: bool = False |
There was a problem hiding this comment.
Can we come up with a better name for this? Perhaps "evaluate_on_live_data"?
There was a problem hiding this comment.
I would rather use the same name as the API to be consistent. Keeping it consistent makes it easier to find and search for.
There was a problem hiding this comment.
Yeah it keeps the update code a bit simpler as well
There was a problem hiding this comment.
The intention of the SiftClient is to abstract the API and its (possibly) confusing or inconsistent implementation. Not overly concerned about this case, but we should not simply match the API topology to be consistent.
There was a problem hiding this comment.
I see, we could go ahead and change it then. I do agree that it's a bit unclear as is
There was a problem hiding this comment.
@alexluck-sift Makes sense. I feel like we should get rid of is_enabled since that's a deprecated field anyway, and just consolidate to is_live. Seems like a bigger change that we don't need to make in this PR right now but should consider in the future to simplify the interface.
There was a problem hiding this comment.
Agreed, if it is deprecated, I don't see a reason to keep it. Seems like a trivial change to include in this PR.
There was a problem hiding this comment.
Yep, I just made those changes
Live rule evaluation was added to UpdateRulesRequest proto here: #424. Updating python class to correspond