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.

Sign up for GitHub

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

Fix the failing domain deletion task #4891

Merged
merged 10 commits into from
Jan 22, 2019
Merged

Fix the failing domain deletion task #4891

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3700ee0
Fix the failing domain deletion task
dojutsu-user Nov 11, 2018
450fd0f
Move import to the top of the file
dojutsu-user Nov 11, 2018
ba068c5
move the imports back
dojutsu-user Nov 11, 2018
8441a0c
Change genertor expression to list comprehension
dojutsu-user Nov 12, 2018
7bb4574
remove list comprehension
dojutsu-user Nov 12, 2018
d2980f4
Revert back the argument name
dojutsu-user Nov 13, 2018
01cf49f
Merge branch 'master' into fix-domain-deletion-task-sym
dojutsu-user Jan 10, 2019
5a1b327
remove dom.domain
dojutsu-user Jan 10, 2019
2d719d3
Merge branch 'master' into fix-domain-deletion-task-sym
dojutsu-user Jan 22, 2019
205e53b
add improvements
dojutsu-user Jan 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions readthedocs/core/symlink.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,10 @@ def symlink_cnames(self, domain=None):
if domain:
domains = [domain]
else:
domains = Domain.objects.filter(project=self.project)
domains = Domain.objects.filter(project=self.project).values_list('domain', flat=True)
for dom in domains:
log_msg = 'Symlinking CNAME: {} -> {}'.format(
dom.domain, self.project.slug
dom, self.project.slug
)
log.info(
constants.LOG_TEMPLATE.format(
Expand All @@ -164,23 +164,23 @@ def symlink_cnames(self, domain=None):
)

# CNAME to doc root
symlink = os.path.join(self.CNAME_ROOT, dom.domain)
symlink = os.path.join(self.CNAME_ROOT, dom)
self.environment.run('ln', '-nsf', self.project_root, symlink)

# Project symlink
project_cname_symlink = os.path.join(
self.PROJECT_CNAME_ROOT, dom.domain
self.PROJECT_CNAME_ROOT, dom
)
self.environment.run(
'ln', '-nsf', self.project.doc_path, project_cname_symlink
)

def remove_symlink_cname(self, domain):
"""Remove CNAME symlink."""
log_msg = "Removing symlink for CNAME {0}".format(domain.domain)
log_msg = "Removing symlink for CNAME {0}".format(domain)
log.info(constants.LOG_TEMPLATE.format(project=self.project.slug,
version='', msg=log_msg))
symlink = os.path.join(self.CNAME_ROOT, domain.domain)
symlink = os.path.join(self.CNAME_ROOT, domain)
safe_unlink(symlink)

def symlink_subprojects(self):
Expand Down
4 changes: 2 additions & 2 deletions readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,12 +972,12 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ
self.domain = parsed.path
super(Domain, self).save(*args, **kwargs)
broadcast(type='app', task=tasks.symlink_domain,
args=[self.project.pk, self.pk],)
args=[self.project.pk, self.domain],)

def delete(self, *args, **kwargs): # pylint: disable=arguments-differ
from readthedocs.projects import tasks
broadcast(type='app', task=tasks.symlink_domain,
args=[self.project.pk, self.pk, True],)
args=[self.project.pk, self.domain, True],)
super(Domain, self).delete(*args, **kwargs)


Expand Down
7 changes: 3 additions & 4 deletions readthedocs/projects/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1043,15 +1043,14 @@ def symlink_project(project_pk):


@app.task(queue='web', throws=(BuildEnvironmentWarning,))
def symlink_domain(project_pk, domain_pk, delete=False):
def symlink_domain(project_pk, domain_str, delete=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note able annotating variable name with type. domain is probably best, as domain_pk doesn't make sense here anymore. Is there a strong reason to change this call either way? Why not change the call to sym.remove_symlink_cname(domain.domain) instead? We have calls from our commercial hosting fork that might break because of the signature change.

Copy link
Member Author

@dojutsu-user dojutsu-user Nov 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@agjohnson
I think we can't call like this sym.remove_symlink_cname(domain.domain) because as mentioned in the issue ( #4789 ).

This task tries to get the Domain object using the pk from the database and it fails because the object was already removed.

So it needs to depend only on the string domain.domain, which is being passed as argument from the 'delete and save method of the 'Domain' class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be worth it to make a similar function for the deletion?
And not touching this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if that is necessary, seems like the current methods are all we need. domain_str can be domain either way however, no need for type annotation on variable names.

project = Project.objects.get(pk=project_pk)
domain = Domain.objects.get(pk=domain_pk)
for symlink in [PublicSymlink, PrivateSymlink]:
sym = symlink(project=project)
if delete:
sym.remove_symlink_cname(domain)
sym.remove_symlink_cname(domain_str)
else:
sym.symlink_cnames(domain)
sym.symlink_cnames(domain_str)


@app.task(queue='web')
Expand Down