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

add timeout for notifications on a per check basis. #40

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

david-d-h
Copy link
Contributor

@david-d-h david-d-h commented Oct 9, 2023

Je kan nu Check::reportTimeout chainen op je check definition om een notification timeout in te stellen.

OK::checks([
    DatabaseTableSizeCheck::config()
        ->setNotificationInterval(now()->subHour()) // een notification kan maar eens in het uur worden verstuurd
        ->setMaxTableSizeInGigabytes([
            'content' => 0.001,
        ]),
]);

@markvaneijk
Copy link
Member

Hi @dulkoss, ik mis nog dat er een default timeout is ingesteld. Dus standaard voor alle checks moet dit x zijn. Dit mag dan aan te passen zijn met een config waarde.

Ook denk ik dat timeout niet helemaal de lading dekt, is setNotificationInterval niet passender?

@markvaneijk
Copy link
Member

markvaneijk commented Oct 10, 2023

Zou je voor deze functie een test kunnen schrijven? Deze zou 't volgende moeten checken:

  • Wanneer bij een check run de 1e fail plaatsvindt, verstuurt de package een notificatie
  • Wanneer direct daarna nog een check run wordt gedraaid (weer een fail) verstuurt deze geen notificatie
  • Vervolgens een time travel (https://laravel-news.com/laravel-time-traveling) naar 15 min later en dan moet de check run wel een notificatie opleveren

Dit kan o.a. getest worden met Notification::fake en ::assert (https://github.com/vormkracht10/laravel-mails/blob/main/tests/NotificationTest.php) en dus de time travel methode

@markvaneijk markvaneijk merged commit 02ea0ee into main Oct 10, 2023
6 of 7 checks passed
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.

2 participants