-
Notifications
You must be signed in to change notification settings - Fork 2
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: Optimus spec based siren alerts #299
base: main
Are you sure you want to change the base?
Conversation
@@ -400,7 +400,7 @@ def get_run_type(context): | |||
def job_success_event(context): | |||
try: | |||
meta = { | |||
"event_type": "TYPE_JOB_SUCCESS", |
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.
is this change expected?
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.
yes this is needed
core/job/spec.go
Outdated
@@ -560,14 +562,31 @@ type WebhookSpec struct { | |||
Endpoints []WebhookEndPoint | |||
} | |||
|
|||
func NewAlertSpec(on string, channels []string, config Config) (*AlertSpec, error) { | |||
const ( | |||
DefaultSeverity = "INFO" |
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.
in here default severity is INFO, while in https://github.com/goto/optimus/pull/299/files#diff-ae8202ecdfcffa6355ad138f9caf0e43b8c5163a4a831e28be335bbcc3a66f2cR28 it is warning. Both should be warning, no?
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.
yes correct,
fixing this
other than the mentioned differences, PR looks good to me |
Proton PR: goto/proton#137