-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Storage updates #5698
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,9 +42,15 @@ | |
from readthedocs.vcs_support.backends import backend_cls | ||
from readthedocs.vcs_support.utils import Lock, NonBlockingLock | ||
|
||
from .constants import ( | ||
MEDIA_TYPES, | ||
MEDIA_TYPE_PDF, | ||
MEDIA_TYPE_EPUB, | ||
MEDIA_TYPE_HTMLZIP, | ||
) | ||
|
||
|
||
log = logging.getLogger(__name__) | ||
storage = get_storage_class()() | ||
|
||
|
||
class ProjectRelationship(models.Model): | ||
|
@@ -463,6 +469,29 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ | |
except Exception: | ||
log.exception('Error creating default branches') | ||
|
||
def delete(self, *args, **kwargs): # pylint: disable=arguments-differ | ||
from readthedocs.projects import tasks | ||
|
||
# Remove local FS build artifacts on the web servers | ||
broadcast( | ||
type='app', | ||
task=tasks.remove_dirs, | ||
args=[(self.doc_path,)], | ||
) | ||
|
||
# Remove build artifacts from storage | ||
storage_paths = [] | ||
for type_ in MEDIA_TYPES: | ||
storage_paths.append( | ||
'{}/{}'.format( | ||
type_, | ||
self.slug, | ||
) | ||
) | ||
tasks.remove_build_storage_paths.delay(storage_paths) | ||
|
||
super().delete(*args, **kwargs) | ||
|
||
def get_absolute_url(self): | ||
return reverse('projects_detail', args=[self.slug]) | ||
|
||
|
@@ -746,32 +775,30 @@ def has_versions(self): | |
def has_aliases(self): | ||
return self.aliases.exists() | ||
|
||
def has_pdf(self, version_slug=LATEST): | ||
def has_media(self, type_, version_slug=LATEST): | ||
path = self.get_production_media_path( | ||
type_='pdf', version_slug=version_slug | ||
) | ||
storage_path = self.get_storage_path( | ||
type_='pdf', version_slug=version_slug | ||
type_=type_, version_slug=version_slug | ||
) | ||
return os.path.exists(path) or storage.exists(storage_path) | ||
if os.path.exists(path): | ||
return True | ||
|
||
if settings.RTD_BUILD_MEDIA_STORAGE: | ||
storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)() | ||
storage_path = self.get_storage_path( | ||
type_=type_, version_slug=version_slug | ||
) | ||
return storage.exists(storage_path) | ||
|
||
return False | ||
|
||
def has_pdf(self, version_slug=LATEST): | ||
return self.has_media(MEDIA_TYPE_PDF, version_slug=version_slug) | ||
|
||
def has_epub(self, version_slug=LATEST): | ||
path = self.get_production_media_path( | ||
type_='epub', version_slug=version_slug | ||
) | ||
storage_path = self.get_storage_path( | ||
type_='epub', version_slug=version_slug | ||
) | ||
return os.path.exists(path) or storage.exists(storage_path) | ||
return self.has_media(MEDIA_TYPE_EPUB, version_slug=version_slug) | ||
|
||
def has_htmlzip(self, version_slug=LATEST): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: these 3 methods are exactly the same; the only change is Not needed to be done in this PR, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems like a worthwhile refactor and it seems useful as part of this PR. |
||
path = self.get_production_media_path( | ||
type_='htmlzip', version_slug=version_slug | ||
) | ||
storage_path = self.get_storage_path( | ||
type_='htmlzip', version_slug=version_slug | ||
) | ||
return os.path.exists(path) or storage.exists(storage_path) | ||
return self.has_media(MEDIA_TYPE_HTMLZIP, version_slug=version_slug) | ||
|
||
@property | ||
def sponsored(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1680,6 +1680,20 @@ def remove_dirs(paths): | |
shutil.rmtree(path, ignore_errors=True) | ||
|
||
|
||
@app.task(queue='web') | ||
def remove_build_storage_paths(paths): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should only be executed by webs typically in response to a project or version being deleted completely. I'll update it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you forgot to push this change. |
||
""" | ||
Remove artifacts from build media storage (cloud or local storage) | ||
|
||
:param paths: list of paths in build media storage to delete | ||
""" | ||
if settings.RTD_BUILD_MEDIA_STORAGE: | ||
storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)() | ||
for storage_path in paths: | ||
log.info('Removing %s from media storage', storage_path) | ||
storage.delete_directory(storage_path) | ||
|
||
|
||
@app.task(queue='web') | ||
def sync_callback(_, version_pk, commit, *args, **kwargs): | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to use either
os.path.join
orpathlib.Path
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.