-
Notifications
You must be signed in to change notification settings - Fork 601
Support manual setting of protocol and base_path #215
Conversation
Failing travis unit test, will reopen after passing |
Now passing lint tests. Fixed a bug with the default use case |
could you squash your commits? it makes it easier to review |
…g optional settings` Squashed commit of the following: commit 401f187 Author: Josh Glick <[email protected]> Date: Tue Feb 10 16:55:52 2015 -0500 Fixed lint formatting issues commit 9c70795 Author: Josh Glick <[email protected]> Date: Tue Feb 10 14:28:17 2015 -0500 Added parens around or if else statement commit e11ec08 Author: Josh Glick <[email protected]> Date: Tue Feb 10 12:28:29 2015 -0500 Fixed a bug with default behaviour when default pass wasn't given commit 3222f25 Author: Josh Glick <[email protected]> Date: Mon Feb 9 16:20:57 2015 -0500 Update full_base_path method to respect protocol property even when base_path not given commit 06875a7 Author: Josh Glick <[email protected]> Date: Mon Feb 9 16:05:11 2015 -0500 Cleaned up scheme / base_path settings to make them cleaner and more distinct commit 575a9ac Author: Josh Glick <[email protected]> Date: Mon Feb 9 15:51:59 2015 -0500 Revert 285a779 commit 285a779 Author: Josh Glick <[email protected]> Date: Mon Feb 9 15:32:01 2015 -0500 Attempt to remove scheme setting commit e754b2e Author: Josh Glick <[email protected]> Date: Mon Feb 9 15:27:52 2015 -0500 Updated Resource View to support no base_path provided commit 8bd8e95 Author: Josh Glick <[email protected]> Date: Mon Feb 9 11:59:03 2015 -0500 Better make use of base_path commit ee6e4a6 Author: Josh Glick <[email protected]> Date: Mon Feb 9 11:54:30 2015 -0500 Updated the discovery url to be based of base_path if provided commit eb37416 Author: Josh Glick <[email protected]> Date: Mon Feb 9 11:45:02 2015 -0500 Attempting to reproduce suggestion from marcgibbons#77 commit 604959a Author: Josh Glick <[email protected]> Date: Mon Feb 9 11:21:36 2015 -0500 Changed how we check for swagger scheme commit b2ed600 Author: Josh Glick <[email protected]> Date: Mon Feb 9 11:13:11 2015 -0500 Support manual setting of scheme
Done! |
could you add protocol and base_path to the docs? |
I have updated the documentation and will take a look at the best way to add tests for this this week. Thank you! |
Any news regarding this pull request? I'm trying to get django-rest-swagger working for this project. However, the default |
Support manual setting of protocol and base_path
Well, that was a bit of a bugger to figure out how to test. |
@@ -109,6 +116,13 @@ def get(self, request): | |||
}), | |||
}) | |||
|
|||
def get_base_path(self): | |||
protocol = SWAGGER_SETTINGS.get('protocol', 'http') |
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.
This patch is breaking backward compatibility because request.is_secure()
is ignored and http
is always used unless I add {'protocol': 'https'}
in my settings.
request.is_secure()
should be the only reliable and consistent way to go here.
I do not understand why adding protocol
in swagger settings is a good idea?
You want to use http
even if request.is_secure() is True
?
If nginx
is forced to connect with http
and if request.is_secure()
return True
, it means you probably have something misconfigured in your settings.
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.
This is also inconsistent with the provided docs. The one case where I was foggily under the impression this would be beneficial is when is_secure is false but you do want https.
While using DRS in our setup, we ran into issue with the protocol and base_path returning the wrong values with our environment.
This was our setup:
Django Application running on EC2 via a Virtual Environment with Gunicorn, NGINX forced to connect over HTTP.
I've added two optional settings for 'protocol' and 'base_path' that can be provided to manually set these fields for people running similar setups to ours.
I believe this might address issues 83 and 77.