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

Upgrade Middleware for Production #3131

Merged
merged 2 commits into from
Jul 26, 2019
Merged

Conversation

rajadain
Copy link
Member

@rajadain rajadain commented Jul 26, 2019

Overview

This was missed in 3a73743, and has caused Staging and Production deploys to fail. Also fixes the health-check endpoint which was failing.

Connects #3130

Demo

http://civicci01.internal.azavea.com/view/mmw/job/model-my-watershed-packer-app-and-worker/1165/console

and

> http --print HhBb :8000/health-check/
GET /health-check/ HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Host: localhost:8000
User-Agent: HTTPie/1.0.2



HTTP/1.1 200 OK
Connection: keep-alive
Content-Encoding: gzip
Content-Type: application/json
Date: Fri, 26 Jul 2019 18:19:44 GMT
Server: nginx
Transfer-Encoding: chunked
Vary: Accept-Encoding

{
    "caches": [
        {
            "default": {
                "ok": true
            }
        }
    ],
    "databases": [
        {
            "default": {
                "ok": true
            }
        }
    ]
}

Testing Instructions

  • Check out this branch
  • Go to :8000/health-check/
    • Ensure it works correctly
  • Go to :8000/health-check (no trailing slash)
    • Ensure you get a ConnectionError, indicating that the middlewares are being skipped correctly (otherwise there is one that appends the trailing slash)

This was missed in 3a73743, and has caused Staging
and Production deploys to fail.
@rajadain rajadain added the WPF Funding Source: William Penn Foundation label Jul 26, 2019
Copy link
Contributor

@jwalgran jwalgran left a comment

Choose a reason for hiding this comment

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

Looks good to me

This was updated for the new style middleware in 3a73743
but was not a complete migration. As of Django 1.10
django/django@05c888f, we need to use _get_response().
Copy link
Contributor

@rbreslow rbreslow left a comment

Choose a reason for hiding this comment

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

I get a 301 when I http --print HhBb :8000/health-check, but this looks good. 👍

@rajadain
Copy link
Member Author

Hmm, so do I actually. The 301 redirects to:

Location: http://localhost/health-check/

so it drops the port and adds a slash. I think that is happening because CommonMIddleware (which adds the trailing slash) preceeds BypassMiddleware, so that still runs.

# MIDDLEWARE CONFIGURATION
# See: https://docs.djangoproject.com/en/1.11/topics/http/middleware/
MIDDLEWARE = (
# Default Django middleware.
'corsheaders.middleware.CorsMiddleware',
'django.middleware.common.CommonMiddleware',
'mmw.middleware.BypassMiddleware',
'django.contrib.sessions.middleware.SessionMiddleware',
'django.middleware.csrf.CsrfViewMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware',
'apps.user.middleware.ItsiAuthenticationMiddleware',
)
# END MIDDLEWARE CONFIGURATION

But the health check is working correctly, which is the main issue. So I'll take that approval and run with it.

@rajadain rajadain merged commit 27da1c5 into develop Jul 26, 2019
@rajadain rajadain deleted the tt/upgrade-django-followup branch July 26, 2019 20:27
@rajadain rajadain assigned rajadain and unassigned rbreslow Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WPF Funding Source: William Penn Foundation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants