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

Serve badges directly from local filesystem #4561

Merged
merged 5 commits into from Aug 27, 2018

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Aug 23, 2018

  • Serves badges directly without a redirect
  • Attaches the cache control header directly to the response as GitHub doesn't respect the cache-control headers on a redirect. GitHub only respects the cache-control header on the 2xx response.
  • A bit of code simplification

This solves an issue that arose because the badges need to be explicitly not cached or have a very short cache TTL. When no cache-control headers are present (as on this badge for example), sometimes other intermediaries -- github in this case -- cache it too long. This PR keeps the cache rules and the response together in the code so this problem doesn't arise again like it did previously in #3323.

Similar to #4559 but reads from a local file instead of possibly remote one.

Fixes #4557

@davidfischer davidfischer requested a review from Aug 23, 2018
Copy link
Member

@ericholscher ericholscher left a comment

I like this approach, just one question.

if version_builds.exists():
last_build = version_builds[0]
if last_build.success:
file_path = static(badge_path % 'passing')
Copy link
Member

@ericholscher ericholscher Aug 23, 2018

Do we still want static(badge_path % 'passing') here instead of file_path = badge_path % 'passing? I thought we wanted to remove the static` call?

Copy link
Contributor Author

@davidfischer davidfischer Aug 23, 2018

We definitely want to get rid of the static call.

version_builds = version.builds.filter(type='html',
state='finished').order_by('-date')
if version_builds.exists():
last_build = version_builds[0]
Copy link
Member

@humitos humitos Aug 23, 2018

nitpick: .first() can be used here.

Copy link
Contributor Author

@davidfischer davidfischer Aug 23, 2018

I suppose if we are cleaning up the existing code a bit, I might as well clean this up too.

# Unknown project
unknown_project_url = reverse('project_badge', args=['fake-project'])
res = self.client.get(unknown_project_url, {'version': 'latest'})
self.assertContains(res, 'unknown')
Copy link
Member

@humitos humitos Aug 23, 2018

nitpick: can we add a check here that the Content-Type header is the correct one, at least in one of these?

Copy link
Contributor Author

@davidfischer davidfischer Aug 23, 2018

Ok

else:
file_path = static(badge_path % 'failing')

with open(file_path) as fd:
Copy link
Member

@humitos humitos Aug 23, 2018

Does it worth here to handle any possible exception when reading the file?

Copy link
Contributor Author

@davidfischer davidfischer Aug 23, 2018

I don't think that is necessary. If Python can't read a local file, we have big problems.

Copy link
Contributor

@agjohnson agjohnson left a comment

Looks good, just some nitpicks


if version:
version_builds = version.builds.filter(type='html',
state='finished').order_by('-date')
Copy link
Contributor

@agjohnson agjohnson Aug 27, 2018

This file was previously autolinted and this line reverts some of the autolinting -- you should be able to resolve with a pre-commit pass.

else:
file_path = badge_path % 'failing'

with open(file_path) as fd:
Copy link
Contributor

@agjohnson agjohnson Aug 27, 2018

Maybe protect with ioerror/oserror catch here? Logic can either return unknown image or at least a 4xx response instead of 5xx.

Copy link
Contributor Author

@davidfischer davidfischer Aug 27, 2018

I think a 5xx response is correct. Maybe a 503 instead of a 500, but definitely not a 4xx. This is a server error and not a client error.

If we can't read from the local filesystem, we can't return the unknown image except with a redirect. How about a 503 instead?

Copy link
Contributor Author

@davidfischer davidfischer Aug 27, 2018

I do wonder whether this is even necessary though. We don't protect against failing to read the local filesystem for Django templates. Is this really any different.

Copy link
Contributor

@agjohnson agjohnson Aug 27, 2018

ah true, i was considering a 4xx response from the perspective of nginx -- if the permissions were wrong or the files went away, nginx would return the respective 4xx response. agree that 5xx is maybe better for this though.

I think my only concern is to log something helpful, and perhaps avoid our default 5xx page if it makes sense.

Copy link
Contributor Author

@davidfischer davidfischer Aug 27, 2018

Let's do a 503 with some logging. That sounds like a plan.

@agjohnson agjohnson added this to the 2.7 milestone Aug 27, 2018
@agjohnson agjohnson merged commit 20e28ca into master Aug 27, 2018
1 check passed
@agjohnson agjohnson deleted the davidfischer/more-badge-changes branch Aug 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants