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

Storage updates #5698

Merged
merged 4 commits into from May 30, 2019
Merged

Storage updates #5698

merged 4 commits into from May 30, 2019

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented May 14, 2019

Since #5549 was merged (although it is not yet deployed) a few issues emerged. This is an attempt to fix them:

  • Delete from storage when a project or version is deleted (Fixes #5691)
  • Project delete now handled in a method on the model (Project.delete())
  • Define media type constants (next step: use them everywhere)
  • A few places still using default storage instead of RTD_BUILD_MEDIA_STORAGE
- Delete from storage when a project or version is deleted
- Project delete now handled in a method on the model
- Define media type constants (next step: use them everywhere)
- A few places still using default storage instead of RTD_BUILD_MEDIA_STORAGE
@davidfischer davidfischer requested a review from May 14, 2019
@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented May 14, 2019

One thing I didn't do in this PR that needs to be done before we turn off all storage on the local webs is to make sure that the code that handles ImportedFiles and search indexing uses storage instead of the local disk.

Copy link
Member

@humitos humitos left a comment

Looks good.

storage_paths = []
for type_ in MEDIA_TYPES:
storage_paths.append(
'{}/{}'.format(
Copy link
Member

@humitos humitos May 15, 2019

I think it's better to use either os.path.join or pathlib.Path here.

Copy link
Contributor Author

@davidfischer davidfischer May 15, 2019

This is not a path on the local disk. This is a storage path which is different. It always uses forward slashes regardless of platform.

Copy link
Member

@humitos humitos May 16, 2019

If we want to go fancy, what we need here is a PurePosixPath: it's platform independent and does not access the filesystem when manipulating paths.

Copy link
Contributor Author

@davidfischer davidfischer May 16, 2019

The Django storage API does not implement a posix standard either. I believe that using pathlib is not the correct course of action because these are not filesystem paths.

)
return storage.exists(storage_path)

return False

def has_htmlzip(self, version_slug=LATEST):
Copy link
Member

@humitos humitos May 15, 2019

nit: these 3 methods are exactly the same; the only change is type_ value. They could be refactored to use the same chunk of code.

Not needed to be done in this PR, though.

Copy link
Contributor Author

@davidfischer davidfischer May 15, 2019

That seems like a worthwhile refactor and it seems useful as part of this PR.

@@ -1680,6 +1680,20 @@ def remove_dirs(paths):
shutil.rmtree(path, ignore_errors=True)


@app.task()
def remove_build_storage_paths(paths):
Copy link
Member

@humitos humitos May 15, 2019

I'm not sure what we decided here, but I think that build servers don't have the credentials (or the permissions) to make this operation. I think this task may be only executed by webs. In that case, queue='web' should be passed in the decorator.

Copy link
Contributor Author

@davidfischer davidfischer May 15, 2019

It should only be executed by webs typically in response to a project or version being deleted completely. I'll update it.

Copy link
Member

@humitos humitos May 16, 2019

I think you forgot to push this change.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented May 15, 2019

I would like to find a better way to test this. It can be a little tricky because many of the functions check both the local disk and storage which in test should probably be the same thing. With that said, there still might be a better approach to testing.

stsewd
stsewd approved these changes May 17, 2019
Copy link
Member

@stsewd stsewd left a comment

LGTM

Copy link
Member

@humitos humitos left a comment

I think this PR is ready to be merged.

@davidfischer davidfischer merged commit c51e96e into master May 30, 2019
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the davidfischer/storage-delete branch May 30, 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.

3 participants