-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Respond directly with badges rather than redirecting #4559
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
Changes from all commits
5162f85
e1f06bf
cfad670
b9abadd
e7ab511
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,11 +15,11 @@ | |
from django.conf import settings | ||
from django.contrib import messages | ||
from django.contrib.auth.models import User | ||
from django.contrib.staticfiles.templatetags.staticfiles import static | ||
from django.core.cache import cache | ||
from django.core.urlresolvers import reverse | ||
from django.http import Http404, HttpResponse, HttpResponseRedirect | ||
from django.shortcuts import get_object_or_404, render | ||
from django.contrib.staticfiles.storage import staticfiles_storage | ||
from django.views.decorators.cache import never_cache | ||
from django.views.generic import DetailView, ListView | ||
from taggit.models import Tag | ||
|
@@ -117,23 +117,30 @@ def project_badge(request, project_slug): | |
style = "flat" | ||
badge_path = 'projects/badges/%s-' + style + '.svg' | ||
version_slug = request.GET.get('version', LATEST) | ||
version = None | ||
url = badge_path % 'unknown' | ||
|
||
try: | ||
version = Version.objects.public(request.user).get( | ||
project__slug=project_slug, slug=version_slug) | ||
except Version.DoesNotExist: | ||
url = static(badge_path % 'unknown') | ||
return HttpResponseRedirect(url) | ||
version_builds = version.builds.filter(type='html', | ||
state='finished').order_by('-date') | ||
if not version_builds.exists(): | ||
url = static(badge_path % 'unknown') | ||
return HttpResponseRedirect(url) | ||
last_build = version_builds[0] | ||
if last_build.success: | ||
url = static(badge_path % 'passing') | ||
else: | ||
url = static(badge_path % 'failing') | ||
return HttpResponseRedirect(url) | ||
pass | ||
|
||
if version: | ||
version_builds = version.builds.filter(type='html', | ||
state='finished').order_by('-date') | ||
if version_builds.exists(): | ||
last_build = version_builds[0] | ||
if last_build.success: | ||
url = badge_path % 'passing' | ||
else: | ||
url = badge_path % 'failing' | ||
|
||
with staticfiles_storage.open(url) as fd: | ||
return HttpResponse( | ||
fd.read(), | ||
content_type='image/svg+xml', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't here the place where we want to add some specific cache headers for lowering down the cache time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method itself has the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perfect! I'm sorry, I didn't see it. |
||
) | ||
|
||
|
||
def project_downloads(request, project_slug): | ||
|
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 there a way to put a timeout on this call? We used to do this exact same thing with
shields.io
, but whenever they had downtime, it took down our entire web stack because we'd end up just spinning our web processes waiting for badges.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.
We weren't doing 302s to shields.io? I knew we used to use shields.io but I didn't look into precisely how it was implemented. Doing redirects seems like it wouldn't rely on them being up. Badges would be down when shields.io is down but it wouldn't affect our servers at all.
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.
It looks like #711 introduced shields.io. It introduced it by streaming the HTTP response from shields which is definitely brittle as you describe. When they're down, it would cause problems for us unless we specify timeouts correctly.
However, it looks like things switched to redirecting to shields.io (in this commit 47757de which doesn't really have a PR). That seems like a step in the right direction for sure. #3057 swapped shields.io for self-hosted SVGs but there's no information in the PR as to why that was desirable. To me it seems like a step back.
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.
To answer this question directly, I don't see a way to add a specific timeout to this call using the version of the Azure libraries we are using. The default timeout is long too (65 seconds).
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.
I believe shields had a decent amount of downtime.
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.
Looks like this is the issue ticket about it: #2065