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

feat: add alert escalation policy as part of the check/config #922

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

ferrandiaz
Copy link
Contributor

I hereby confirm that I followed the code guidelines found at engineering guidelines

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Alert Escalation policy can be defined either on the check, the check group or the checkly config file

Resolves #901

@ferrandiaz
Copy link
Contributor Author

I see one test failing due to Unable to run checks: \"checkRunJobs[0].useGlobalAlertSettings\" is not allowed , I suspect this is going to be fixed the moment I merge this other PR https://github.com/checkly/checkly-backend/pull/5075

Comment on lines +173 to +174
this.alertSettings = props.alertEscalationPolicy
this.useGlobalAlertSettings = !this.alertSettings
Copy link
Member

Choose a reason for hiding this comment

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

@tnolet what do you think about this? Basically we set useGlobalAlertSettings to true if alertSettings doesn't exist and vice versa

Copy link
Member

Choose a reason for hiding this comment

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

@umutuzgur @ferrandiaz hmmmm, feels a bit iffy / hidden magic. Not sure if we have a great alternative.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just my 2 cents, but I think it's nice. As a user, I think that it would be confusing if I passed alertSettings, but they weren't actually applied because I need to also explicitly set useGlobalAlertSettings. I'm not sure there's a case in the CLI were I would pass alertSettings but not want it to actually be used.

Copy link
Member

Choose a reason for hiding this comment

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

@clample yes, I agree. This seems to be the best UX. @umutuzgur @ferrandiaz let's go with this.

@ferrandiaz ferrandiaz force-pushed the ferran/alert-escalation-policy branch from e497cd1 to 542631f Compare January 16, 2024 11:02
@ferrandiaz ferrandiaz merged commit 1857918 into main Jan 16, 2024
3 checks passed
@ferrandiaz ferrandiaz deleted the ferran/alert-escalation-policy branch January 16, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support configuring alert escalation policies
4 participants