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

Enable default HTTPS support #86

Merged
merged 11 commits into from Oct 4, 2018

Conversation

Projects
None yet
4 participants
@marcosguedes
Contributor

marcosguedes commented Apr 6, 2018

All supplied backends support HTTPS. I think it's fine to support HTTPS by default

Marcos Guedes
@joelschopp

This comment has been minimized.

joelschopp commented Apr 6, 2018

Agree, we should switch to https being default

CedricCarrard added some commits Aug 31, 2018

@CedricCarrard

This comment has been minimized.

Member

CedricCarrard commented Aug 31, 2018

@marcosguedes Please look to correct the tests thanks

@marcosguedes

This comment has been minimized.

Contributor

marcosguedes commented Aug 31, 2018

@CedricCarrard thanks for pointing that out, I'll handle it asap

@marcosguedes

This comment has been minimized.

Contributor

marcosguedes commented Aug 31, 2018

@CedricCarrard Fixed. Apologies for the wait, I've never used tests like these before, I've learned a lot :)

@marcosguedes

This comment has been minimized.

Contributor

marcosguedes commented Aug 31, 2018

@marcosguedes

This comment has been minimized.

Contributor

marcosguedes commented Aug 31, 2018

How odd... it retrieved a vimeo thumbnail on http after requesting https on Django==1.10.x . I've tested it 5 times and it was a success everytime. Is this a vimeo issue?

@mgrdcm

This comment has been minimized.

Member

mgrdcm commented Sep 11, 2018

Hey!

Even if secure/SSL is the default, not sure we'd want to remove any of the tests for http URLs, since those are still valid too.

@CedricCarrard

This comment has been minimized.

Member

CedricCarrard commented Sep 28, 2018

@mgrdcm Would you have time to help @marcosguedes finish the pull-request? I'm waiting on the pull request to create a new version.

@mgrdcm

This comment has been minimized.

Member

mgrdcm commented Sep 28, 2018

I can, but might be a day or two...

@marcosguedes

This comment has been minimized.

Contributor

marcosguedes commented Sep 28, 2018

Hi @CedricCarrard @mgrdcm so sorry, heavy workload these past days and I completely forgot. Will try again today in about 5h.

Cheers and thank you for your patience!

@marcosguedes

This comment has been minimized.

Contributor

marcosguedes commented Oct 3, 2018

Hi there @mgrdcm , @CedricCarrard ! Apologies for being late. Is it ok right now? Not sure if I git-reverted successfully

@mgrdcm mgrdcm self-requested a review Oct 3, 2018

@mgrdcm

mgrdcm approved these changes Oct 3, 2018

@CedricCarrard this looks good to me! Seems to pass tests and works in one of my apps that uses this.

I was initially a little concerned about is_secure = True being set individually for the three builtin VideoBackend subclasses, but I think that makes more sense than setting in VideoBackend directly because that'd probably be considered a breaking change.

Good work, @marcosguedes!

@mgrdcm

This comment has been minimized.

Member

mgrdcm commented Oct 3, 2018

@CedricCarrard I can merge this if you prefer, otherwise leaving it for you to make final call. Thanks!

@CedricCarrard

This comment has been minimized.

Member

CedricCarrard commented Oct 4, 2018

@mgrdcm @marcosguedes good work guys thanks.

@CedricCarrard CedricCarrard merged commit ed53ef8 into jazzband:master Oct 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment