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

Fire a signal for domain verification (eg. for SSL) #5071

Merged
merged 2 commits into from Jan 15, 2019

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Jan 4, 2019

We have had a few issues where domains have taken a long time to issue SSL certificates (example #4736 (comment)). Cloudflare -- who provides our SSL certs for custom domains -- automatically retries verification requests but they have an exponential backoff and then they stop retrying after a couple weeks. That means that if you add your domain to RTD and then don't fix your DNS for a day or two, it can take days to verify your domain. Users could of course remove the domain from RTD and re-add it but few people seem to do that.

This PR fires a task which fires signals which are handled in the cloudflare code (not in the readthedocs.org repo) to automatically retry verification whenever somebody visits the domain list screen (/dashboard/<project_slug>/domains/)

@davidfischer davidfischer requested a review from Jan 4, 2019
humitos
humitos approved these changes Jan 7, 2019
Copy link
Member

@humitos humitos left a comment

I like the idea!

I left two comments about minor things to be improved if you consider.

@@ -1393,3 +1394,17 @@ def finish_inactive_builds():
'Builds marked as "Terminated due inactivity": %s',
builds_finished,
)


@app.task
Copy link
Member

@humitos humitos Jan 7, 2019

I suppose that you should pass queue='web' since we are accessing the database from inside the task.

@@ -726,7 +727,14 @@ def get_success_url(self):


class DomainList(DomainMixin, ListViewWithForm):
pass
def get_context_data(self, **kwargs):
Copy link
Member

@humitos humitos Jan 7, 2019

I think it's better to put this code under get method, don't you think?

Copy link
Contributor Author

@davidfischer davidfischer Jan 10, 2019

I think that would be worse, actually. We need access to the domain_list from the context. The get method both calls get_context_data and consumes the context (but doesn't return it). As a result, we'd have to replace get and not merely call the super method and add functionality. Does that make sense?

Copy link
Member

@ericholscher ericholscher left a comment

Looks good after the update to the web queue.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 10, 2019

Pushed a commit to fix my only issue. This is 👍 for shipping.

@ericholscher ericholscher merged commit f28c236 into master Jan 15, 2019
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the davidfischer/domain-verification-retry branch Jan 15, 2019
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

3 participants