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

Project updated when subproject modified #3649

Merged
merged 7 commits into from Mar 9, 2018

Conversation

Projects
None yet
3 participants
@humitos
Member

humitos commented Feb 22, 2018

Closes #3396

To Do:

  • find the correct before/after filesystem
  • write the test in a proper way
  • make the test fail/pass manually
  • implement the solution proposed in the original issue

humitos added some commits Feb 28, 2018

Trigger re-symlink for superproject when project changes
Re-symlink when:

* a subproject is deleted
* a subproject privacy level is changed
* a subproject version privacy level is changed
Move logic from Form to Model
Instead of trigger the re-symlink task on each of the Form actions, we
trigger it once on ``Project.save()`` or ``Project.delete()`` method.
@humitos

This comment has been minimized.

Member

humitos commented Mar 6, 2018

I have some questions/doubts that are related to this PR (maybe to be implemented in another -but related):

  • when a Version is deleted, we are triggering clear_artifacts and symlink_project but the later is triggered before object deletion; so, I think if the task is ran before it's effectively deleted the symlink won't be properly synced:

def delete(self, *args, **kwargs): # pylint: disable=arguments-differ
from readthedocs.projects import tasks
log.info('Removing files for version %s', self.slug)
broadcast(type='app', task=tasks.clear_artifacts, args=[self.pk])
broadcast(
type='app', task=tasks.symlink_project, args=[self.project.pk])
super(Version, self).delete(*args, **kwargs)

  • when a (sub)Project is deleted we don't sync anything. I think that maybe we should sync the superprojects

  • when a (super)Project is deleted, what should happen with the subprojects?

@agjohnson the PR for the original issue is ready to review --let me know if you want me to squash the commits into one. Although, I would like you to take a look a those questions that I'm raising here.

@ericholscher

This comment has been minimized.

Member

ericholscher commented Mar 7, 2018

There are a lot of bugs in our "keep symlinks up to date" code. We should write a design document that specifies the exact place that we should be doing sync'ing for this, and then do an audit (and tests!) to make sure that we're actually doing it.

I've tried to build a proper abstraction for maintaining the symlinks in code (https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/core/symlink.py), but that never got pushed back into a proper abstraction for updating/removing/editing symlinks with the lifecycle of the model. I think that is what this PR (and your questions) are addressing.

"What is the correct way to handle updating & syncing symlinks when models change?" is the question we need to answer.

@agjohnson

The test that you added is a good litmus test for this particular change, but we should add a test that ensures the we're calling the broadcast functions correctly in save().

For instance:

  • Symlink broadcast is called as expected
  • Symlink broadcast is called for superprojects in the case that we have superprojects and don't have superprojects
@@ -9,3 +9,4 @@ Mercurial==4.4.2
# local debugging tools
pdbpp
datadiff

This comment has been minimized.

@agjohnson

agjohnson Mar 7, 2018

Contributor

looks unused, but is it helpful to keep around?

This comment has been minimized.

@humitos

humitos Mar 8, 2018

Member

I used a lot while debugging to diff two dicts (current filesystem and expected one). It's a debugging tool that I usually use, but we can definetly remove from here if you want.

This comment has been minimized.

@agjohnson

agjohnson Mar 9, 2018

Contributor

great, let's keep it for debug purposes

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Mar 7, 2018

A design document might be good to have in the future, but i think will slow down this work without much gain to be had. I'll create a new issue for this under our refactoring milestone.

@humitos

This comment has been minimized.

Member

humitos commented Mar 9, 2018

@agjohnson I just pushed a change with a test to check that broadcast function was called as it's expected. Let me know if that is what you were thinking.

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Mar 9, 2018

Looks great!

@agjohnson agjohnson merged commit cc18a75 into master Mar 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@humitos humitos deleted the humitos/subproject/update-superproject branch Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment