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

Fix the failing domain deletion task #4891

Merged
merged 10 commits into from
Jan 22, 2019
Merged

Fix the failing domain deletion task #4891

merged 10 commits into from
Jan 22, 2019

Conversation

dojutsu-user
Copy link
Member

Fixes #4789

@codecov
Copy link

codecov bot commented Nov 11, 2018

Codecov Report

Merging #4891 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4891      +/-   ##
==========================================
- Coverage   76.87%   76.65%   -0.23%     
==========================================
  Files         158      158              
  Lines        9997    10056      +59     
  Branches     1255     1269      +14     
==========================================
+ Hits         7685     7708      +23     
- Misses       1980     2007      +27     
- Partials      332      341       +9
Impacted Files Coverage Δ
readthedocs/projects/models.py 85.44% <ø> (-0.52%) ⬇️
readthedocs/projects/tasks.py 70.96% <100%> (+0.08%) ⬆️
readthedocs/core/symlink.py 94.47% <100%> (-0.04%) ⬇️
readthedocs/vcs_support/backends/svn.py 27.69% <0%> (-11.71%) ⬇️
readthedocs/vcs_support/backends/hg.py 59.09% <0%> (-8.61%) ⬇️
readthedocs/core/utils/__init__.py 74.73% <0%> (-5.68%) ⬇️
readthedocs/projects/utils.py 54.54% <0%> (-4.55%) ⬇️
readthedocs/restapi/views/integrations.py 88.65% <0%> (-3.31%) ⬇️
readthedocs/vcs_support/backends/git.py 80.35% <0%> (-2.41%) ⬇️
readthedocs/vcs_support/base.py 88.67% <0%> (-1.89%) ⬇️
... and 27 more

@dojutsu-user
Copy link
Member Author

@humitos Please review.

Copy link
Member

@humitos humitos 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. Thanks!

I just left a question about a refactor that I think it's not needed.

readthedocs/core/symlink.py Outdated Show resolved Hide resolved
readthedocs/core/symlink.py Outdated Show resolved Hide resolved
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

I'd prefer this doesn't change function signatures, which seems mostly avoidable. Our commercial hosting code also has code that calls some of these tasks, and the change here would require cleanup on that codebase as well.

readthedocs/core/symlink.py Outdated Show resolved Hide resolved
@@ -1002,15 +1002,14 @@ def symlink_project(project_pk):


@app.task(queue='web')
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

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.

@agjohnson agjohnson added this to the 2.9 milestone Nov 13, 2018
@humitos
Copy link
Member

humitos commented Jan 10, 2019

This PR has some conflicts to resolve and I think it's close to be merged. It would be good to fix these and merge.

@dojutsu-user
Copy link
Member Author

@humitos
I have resolved the conflicts.
But I think it is better to wait for @agjohnson to review this again because of his comment above.

@agjohnson agjohnson added the Bug A bug label Jan 22, 2019
@agjohnson agjohnson dismissed their stale review January 22, 2019 01:27

Variable change unaddressed

@agjohnson
Copy link
Contributor

Bleh, github doesn't know what it's doing. Tried to dismiss requested review on me and it rejects my previous review 👍

Still blocking on variable name nitpick. Update there makes this mergable

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

I already reviewed this github! 👍

@dojutsu-user
Copy link
Member Author

@agjohnson
I have changed domain_str to domain and also added function doc strings to avoid confusion on what exactly is domain.

@ericholscher
Copy link
Member

Looks good. 👍

@ericholscher ericholscher merged commit 4ee4f61 into readthedocs:master Jan 22, 2019
@dojutsu-user dojutsu-user deleted the fix-domain-deletion-task-sym branch January 22, 2019 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants