Adjust notifications IDL to support notifications per phases#7025
Adjust notifications IDL to support notifications per phases#7025iaroslav-ciupin merged 8 commits intov2from
Conversation
Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
| option (buf.validate.message).cel = { | ||
| id: "at_least_one_required" | ||
| message: "at least one of the delivery options must be set" | ||
| expression: "has(this.webhook) || has(this.email)" | ||
| }; |
There was a problem hiding this comment.
Because in oneof you must have at most one. Here you can have as many notifications as you want. for same phase match, we may want both email & webhook & Slack for example.
| message DeliveryConfigId { | ||
| // Org this rule belongs to. | ||
| // Org this config belongs to. | ||
| string org = 1 [ |
There was a problem hiding this comment.
why keep org? We should just make all of this at the rule level no?
There was a problem hiding this comment.
I don't follow, can you elaborate?
this is a separate entity that lives in its own table, in my understanding org is needed:
- for authz, to ensure you are referring to delivery config set up by your org, and not someone else's
- to avoid name collision in DB, so that you don't accidentally update/delete someone else's config.
There was a problem hiding this comment.
When you register a notification the runspec or the trigger already has the org. Why would you ever lookup some other org. We should drop it and infer from the context
There was a problem hiding this comment.
@kumare3 you are mixing one-off settings and persisted settings.
One-off settings are stored in runspec via CreateRun or DeployTask(with triggers) endpoints.
Persisted settings are stored independently of any tasks, runs, triggers, etc. Its a separate set of endpoints to create a Rule and a DeliveryConfig(see commit where I removed this API 75c97de )
There was a problem hiding this comment.
I think what Ketan is probably saying is that in the RunSpec, instead of embedding this id, you just need the "string name" of the DeliveryConfig...
There was a problem hiding this comment.
ah, I thought he was referring to removing org from DeliveryConfigId object.
Yeah, we can infer org from a run/trigger when creating a one-off notification setting. 👍
Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
|
implemented this IDL on backend https://github.com/unionai/cloud/pull/14878 |
Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
| // Optionally, you can send a notification when your run completes. | ||
| oneof notification_settings { | ||
| flyteidl2.notification.RuleId rule_id = 10; // either refer to a previously stored notification rule | ||
| string notification_rule_name = 10; // either refer to a previously stored notification rule |
* adjust notifications IDL to support notifications per phases * drop delivery config id & rule id * email idl adjustments Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
3 participants
Tracking issue
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
main