-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(uptime): Enforce per-domain limits #77857
feat(uptime): Enforce per-domain limits #77857
Conversation
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
e87a73f
to
23f74fb
Compare
23f74fb
to
7707af6
Compare
url_parts = extract_domain_parts(url) | ||
existing_count = ProjectUptimeSubscription.objects.filter( | ||
uptime_subscription__url_domain=url_parts.domain, | ||
uptime_subscription__url_domain_suffix=url_parts.suffix, | ||
).count() |
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.
One thought here is that maybe this should happen specifically in the creation function. Since we have the de-duping, it's valid to create a monitor on an existing url even if we're at the limit, since it doesn't cause any more hits to that url.
Maybe the create function could raise an exception that we catch and translate in save?
Maybe also just an edge case and not a big deal
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.
oh right, hmm. But you can edit a monitor to change the url as well right. Yeah I guess it could be an exception.
Let's not worry about it for now.
Need to fix the test, but this will make it so you can no longer create
more monitors once the per domain limit is reached