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

pre-commit command ran for humitos/log/sentry branch #3898

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions .style.yapf
2 changes: 1 addition & 1 deletion readthedocs-common
9 changes: 5 additions & 4 deletions readthedocs/builds/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@
import logging
import os.path
import re
from builtins import object
from shutil import rmtree

from builtins import object
from django.conf import settings
from django.core.urlresolvers import reverse
from django.db import models
from django.utils.encoding import python_2_unicode_compatible
from django.utils.translation import ugettext_lazy as _
from django.utils.translation import ugettext
from django.utils.translation import ugettext_lazy as _
from guardian.shortcuts import assign
from taggit.managers import TaggableManager

Expand Down Expand Up @@ -148,7 +148,8 @@ def commit_name(self):

# If we came that far it's not a special version nor a branch or tag.
# Therefore just return the identifier to make a safe guess.
log.debug('TODO: Raise an exception here. Testing what cases it happens')
log.debug(
'TODO: Raise an exception here. Testing what cases it happens')
return self.identifier

def get_absolute_url(self):
Expand Down Expand Up @@ -247,7 +248,7 @@ def clean_build_path(self):
try:
path = self.get_build_path()
if path is not None:
log.debug('Removing build path {0} for {1}'.format(path, self))
log.debug('Removing build path %s for %s', path, self)
rmtree(path)
except OSError:
log.exception('Build path cleanup failed')
Expand Down
59 changes: 27 additions & 32 deletions readthedocs/builds/syncers.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
# -*- coding: utf-8 -*-
"""
Classes to copy files between build and web servers.

"Syncers" copy files from the local machine, while "Pullers" copy files to the
local machine.
"""

from __future__ import absolute_import
from __future__ import (
absolute_import, division, print_function, unicode_literals)

import getpass
import logging
Expand All @@ -15,9 +17,8 @@
from builtins import object
from django.conf import settings

from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.core.utils import safe_makedirs

from readthedocs.core.utils.extend import SettingsOverrideObject

log = logging.getLogger(__name__)

Expand All @@ -27,7 +28,7 @@ class LocalSyncer(object):
@classmethod
def copy(cls, path, target, is_file=False, **__):
"""A copy command that works with files or directories."""
log.info("Local Copy %s to %s", path, target)
log.info('Local Copy %s to %s', path, target)
if is_file:
if path == target:
# Don't copy the same file over itself
Expand All @@ -53,30 +54,26 @@ def copy(cls, path, target, is_file=False, **__):
sync_user = getattr(settings, 'SYNC_USER', getpass.getuser())
app_servers = getattr(settings, 'MULTIPLE_APP_SERVERS', [])
if app_servers:
log.info("Remote Copy %s to %s on %s", path, target, app_servers)
log.info('Remote Copy %s to %s on %s', path, target, app_servers)
for server in app_servers:
mkdir_cmd = ("ssh %s@%s mkdir -p %s" % (sync_user, server, target))
mkdir_cmd = (
'ssh %s@%s mkdir -p %s' % (sync_user, server, target))
ret = os.system(mkdir_cmd)
if ret != 0:
log.info("COPY ERROR to app servers:")
log.info(mkdir_cmd)
log.error('Copy error to app servers: cmd=%s', mkdir_cmd)
if is_file:
slash = ""
slash = ''
else:
slash = "/"
slash = '/'
# Add a slash when copying directories
sync_cmd = (
"rsync -e 'ssh -T' -av --delete {path}{slash} {user}@{server}:{target}"
.format(
path=path,
slash=slash,
user=sync_user,
server=server,
path=path, slash=slash, user=sync_user, server=server,
Copy link
Contributor

Choose a reason for hiding this comment

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

This squashed the continuation

Copy link
Member Author

Choose a reason for hiding this comment

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

I had sent a comment here, but my internet connection didn't help me:


By adding a , after the target here, we get a one attribute per line formatting automatically by yapf.

There are many changes on these files were we can apply this fix to get the more readable way, but I don't want to do it manually for now.

I think we can easily add the trailing comman once we start using pre-commit in a daily basis.


Anyway, I think we can use this PR to discuss/tweak yapf a little more.

There are things that need manual work. Although, for this particular issue that the trailling comma is missing we could add automatically before calling yapf with this hook: https://github.com/asottile/add-trailing-comma

target=target))
ret = os.system(sync_cmd)
if ret != 0:
log.info("COPY ERROR to app servers.")
log.info(sync_cmd)
log.error('Copy error to app servers: cmd=%s', sync_cmd)


class DoubleRemotePuller(object):
Expand All @@ -91,31 +88,25 @@ def copy(cls, path, target, host, is_file=False, **__):
sync_user = getattr(settings, 'SYNC_USER', getpass.getuser())
app_servers = getattr(settings, 'MULTIPLE_APP_SERVERS', [])
if not is_file:
path += "/"
log.info("Remote Copy %s to %s", path, target)
path += '/'
log.info('Remote Copy %s to %s', path, target)
for server in app_servers:
if not is_file:
mkdir_cmd = "ssh {user}@{server} mkdir -p {target}".format(
user=sync_user, server=server, target=target
)
mkdir_cmd = 'ssh {user}@{server} mkdir -p {target}'.format(
user=sync_user, server=server, target=target)
ret = os.system(mkdir_cmd)
if ret != 0:
log.info("MKDIR ERROR to app servers:")
log.info(mkdir_cmd)
log.error('MkDir error to app servers: cmd=%s', mkdir_cmd)
# Add a slash when copying directories
sync_cmd = (
"ssh {user}@{server} 'rsync -av "
"--delete --exclude projects {user}@{host}:{path} {target}'"
.format(
host=host,
path=path,
user=sync_user,
server=server,
host=host, path=path, user=sync_user, server=server,
Copy link
Contributor

Choose a reason for hiding this comment

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

Squashed continuation here

target=target))
ret = os.system(sync_cmd)
if ret != 0:
log.info("COPY ERROR to app servers.")
log.info(sync_cmd)
log.error('Copy error to app servers: cmd=%s', sync_cmd)


class RemotePuller(object):
Expand All @@ -129,8 +120,8 @@ def copy(cls, path, target, host, is_file=False, **__):
"""
sync_user = getattr(settings, 'SYNC_USER', getpass.getuser())
if not is_file:
path += "/"
log.info("Remote Pull %s to %s", path, target)
path += '/'
log.info('Remote Pull %s to %s', path, target)
if not is_file and not os.path.exists(target):
safe_makedirs(target)
# Add a slash when copying directories
Expand All @@ -142,7 +133,11 @@ def copy(cls, path, target, host, is_file=False, **__):
)
ret = os.system(sync_cmd)
if ret != 0:
log.error("COPY ERROR to app servers. Command: [{}] Return: [{}]".format(sync_cmd, ret))
log.error(
'Copy error to app servers. Command: [%s] Return: [%s]',
sync_cmd,
ret,
)


class Syncer(SettingsOverrideObject):
Expand Down
50 changes: 27 additions & 23 deletions readthedocs/core/management/commands/clean_builds.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
"""Clean up stable build paths per project version"""
# -*- coding: utf-8 -*-
"""Clean up stable build paths per project version."""

from __future__ import (
absolute_import, division, print_function, unicode_literals)

from __future__ import absolute_import
from datetime import datetime, timedelta
import logging
from datetime import datetime, timedelta
from optparse import make_option

from django.core.management.base import BaseCommand
Expand All @@ -18,40 +21,41 @@ class Command(BaseCommand):
help = __doc__

option_list = BaseCommand.option_list + (
make_option('--days',
dest='days',
type='int',
default=365,
help='Find builds older than DAYS days, default: 365'),
make_option('--dryrun',
action='store_true',
dest='dryrun',
help='Perform dry run on build cleanup'),
make_option(
'--days', dest='days', type='int', default=365,
help='Find builds older than DAYS days, default: 365'),
make_option(
'--dryrun', action='store_true', dest='dryrun',
help='Perform dry run on build cleanup'),
)

def handle(self, *args, **options):
"""Find stale builds and remove build paths"""
"""Find stale builds and remove build paths."""
max_date = datetime.now() - timedelta(days=options['days'])
queryset = (Build.objects
.values('project', 'version')
.annotate(max_date=Max('date'))
.filter(max_date__lt=max_date)
.order_by('-max_date'))
queryset = (
Build.objects.values('project',
'version').annotate(max_date=Max('date'))
.filter(max_date__lt=max_date).order_by('-max_date'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Previous continuation style was better

for build in queryset:
try:
# Get version from build version id, perform sanity check on
# latest build date
version = Version.objects.get(id=build['version'])
latest_build = version.builds.latest('date')
if latest_build.date > max_date:
log.warning('{0} is newer than {1}'.format(
latest_build, max_date))
log.warning(
'%s is newer than %s',
latest_build,
max_date,
)
path = version.get_build_path()
if path is not None:
log.info(
('Found stale build path for {0} '
'at {1}, last used on {2}').format(
version, path, latest_build.date))
'Found stale build path for %s at %s, last used on %s',
version,
path,
latest_build.date,
)
if not options['dryrun']:
version.clean_build_path()
except Version.DoesNotExist:
Expand Down
27 changes: 12 additions & 15 deletions readthedocs/core/management/commands/reindex_elasticsearch.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
"""Reindex Elastic Search indexes"""
# -*- coding: utf-8 -*-
"""Reindex Elastic Search indexes."""

from __future__ import (
absolute_import, division, print_function, unicode_literals)

from __future__ import absolute_import
import logging
from optparse import make_option

from django.core.management.base import BaseCommand
from django.core.management.base import CommandError
from django.conf import settings
from django.core.management.base import BaseCommand, CommandError

from readthedocs.builds.constants import LATEST
from readthedocs.builds.models import Version
Expand All @@ -19,14 +21,10 @@ class Command(BaseCommand):

help = __doc__
option_list = BaseCommand.option_list + (
make_option('-p',
dest='project',
default='',
help='Project to index'),
)
make_option('-p', dest='project', default='', help='Project to index'),)

def handle(self, *args, **options):
"""Build/index all versions or a single project's version"""
"""Build/index all versions or a single project's version."""
project = options['project']

queryset = Version.objects.all()
Expand All @@ -36,12 +34,12 @@ def handle(self, *args, **options):
if not queryset.exists():
raise CommandError(
'No project with slug: {slug}'.format(slug=project))
log.info("Building all versions for %s", project)
log.info('Building all versions for %s', project)
elif getattr(settings, 'INDEX_ONLY_LATEST', True):
queryset = queryset.filter(slug=LATEST)

for version in queryset:
log.info("Reindexing %s", version)
log.info('Reindexing %s', version)
try:
commit = version.project.vcs_repo(version.slug).commit
except: # pylint: disable=bare-except
Expand All @@ -50,7 +48,6 @@ def handle(self, *args, **options):
commit = None

try:
update_search(version.pk, commit,
delete_non_commit_files=False)
update_search(version.pk, commit, delete_non_commit_files=False)
except Exception:
log.exception('Reindex failed for {}'.format(version))
log.exception('Reindex failed for %s', version)
30 changes: 15 additions & 15 deletions readthedocs/core/signals.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
# -*- coding: utf-8 -*-
"""Signal handling for core app."""

from __future__ import absolute_import
from __future__ import (
absolute_import, division, print_function, unicode_literals)

import logging

from corsheaders import signals
from django.conf import settings
from django.db.models import Count, Q
from django.db.models.signals import pre_delete
from django.dispatch import Signal
from django.db.models import Q, Count
from django.dispatch import receiver
from django.dispatch import Signal, receiver
from future.backports.urllib.parse import urlparse

from readthedocs.projects.models import Project, Domain
from readthedocs.projects.models import Domain, Project

log = logging.getLogger(__name__)

Expand All @@ -23,7 +24,6 @@
'/api/v2/sustainability',
]


webhook_github = Signal(providing_args=['project', 'data', 'event'])
webhook_gitlab = Signal(providing_args=['project', 'data', 'event'])
webhook_bitbucket = Signal(providing_args=['project', 'data', 'event'])
Expand Down Expand Up @@ -57,17 +57,15 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen
project = Project.objects.get(slug=project_slug)
except Project.DoesNotExist:
log.warning(
'Invalid project passed to domain. [{project}:{domain}'.format(
project=project_slug,
domain=host,
)
'Invalid project passed to domain. [%s:%s]',
project_slug,
host,
)
return False

domain = Domain.objects.filter(
Q(domain__icontains=host),
Q(project=project) | Q(project__subprojects__child=project)
)
Q(project=project) | Q(project__subprojects__child=project))
if domain.exists():
return True

Expand All @@ -78,12 +76,14 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen
def delete_projects_and_organizations(sender, instance, *args, **kwargs):
# Here we count the owner list from the projects that the user own
# Then exclude the projects where there are more than one owner
projects = instance.projects.all().annotate(num_users=Count('users')).exclude(num_users__gt=1)
projects = instance.projects.all().annotate(
num_users=Count('users')).exclude(num_users__gt=1)

# Here we count the users list from the organization that the user belong
# Then exclude the organizations where there are more than one user
oauth_organizations = (instance.oauth_organizations.annotate(num_users=Count('users'))
.exclude(num_users__gt=1))
oauth_organizations = (
instance.oauth_organizations.annotate(num_users=Count('users'))
.exclude(num_users__gt=1))

projects.delete()
oauth_organizations.delete()
Expand Down
Loading