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

Add models for automation rules #5323

Merged
merged 38 commits into from May 21, 2019
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
2ae80d1
Add models for automation rules
stsewd Feb 20, 2019
d0e04e0
Merge branch 'master' into add-models-for-automationrules
stsewd Feb 28, 2019
d53bad2
Use django-polymorphic with proxy models
stsewd Feb 28, 2019
d287286
Add migration
stsewd Feb 28, 2019
c5e6010
Merge branch 'master' into add-models-for-automationrules
stsewd Mar 4, 2019
cb5e1b9
Refactor and suggestions from review
stsewd Mar 4, 2019
4fd56f6
Update migration
stsewd Mar 4, 2019
e570a58
Implement activation action
stsewd Mar 4, 2019
353bc8a
Clean model
stsewd Mar 4, 2019
7a60668
Tests!
stsewd Mar 4, 2019
409e960
Refactor name
stsewd Mar 4, 2019
8f58e77
Refactor models
stsewd Mar 4, 2019
8dfe290
Integrate in admin
stsewd Mar 4, 2019
0d4ab95
Merge branch 'master' into add-models-for-automationrules
stsewd Mar 6, 2019
74f1b9a
Don't search for project.name
stsewd Mar 6, 2019
7936f55
Merge branch 'master' into add-models-for-automationrules
stsewd Apr 1, 2019
4c77043
Merge branch 'master' into add-models-for-automationrules
stsewd Apr 25, 2019
11add5d
Return explicitly a boolean
stsewd Apr 25, 2019
5e90f62
Use action_arg from self
stsewd Apr 25, 2019
0903763
Add set default version action
stsewd Apr 25, 2019
26fc59b
Fix migrations
stsewd Apr 25, 2019
7660b86
Typo
stsewd Apr 26, 2019
a2c50ac
Execute automation rules on new versions
stsewd Apr 26, 2019
0f06958
Typo
stsewd Apr 26, 2019
bf2f954
Trigger build after activate
stsewd Apr 26, 2019
73030c0
Better admin page
stsewd Apr 29, 2019
6237ca4
More tests
stsewd Apr 29, 2019
25faa28
More tests
stsewd Apr 29, 2019
eda1cd2
Test from api call
stsewd Apr 29, 2019
39c8cc5
Tests for set_default_version
stsewd Apr 29, 2019
a6d959d
Fix test
stsewd Apr 29, 2019
2ae831d
Docs
stsewd Apr 29, 2019
78e6e62
Period
stsewd Apr 29, 2019
5d0c706
Merge branch 'master' into add-models-for-automationrules
stsewd Apr 29, 2019
b2a1339
Merge branch 'master' into add-models-for-automationrules
stsewd May 9, 2019
b07b660
Notes about the versions order
stsewd May 9, 2019
a86d427
Merge branch 'master' into add-models-for-automationrules
stsewd May 20, 2019
cbce26a
Fix mock
stsewd May 20, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 31 additions & 6 deletions readthedocs/builds/admin.py
@@ -1,15 +1,23 @@
# -*- coding: utf-8 -*-

"""Django admin interface for `~builds.models.Build` and related models."""

from django.contrib import admin, messages
from guardian.admin import GuardedModelAdmin

from readthedocs.builds.models import Build, BuildCommandResult, Version
from polymorphic.admin import (
PolymorphicChildModelAdmin,
PolymorphicParentModelAdmin,
)

from readthedocs.builds.models import (
Build,
BuildCommandResult,
RegexAutomationRule,
Version,
VersionAutomationRule,
)
from readthedocs.core.utils import trigger_build
from readthedocs.core.utils.general import wipe_version_via_slugs
from readthedocs.projects.models import HTMLFile
from readthedocs.search.utils import _indexing_helper
from readthedocs.core.utils.general import wipe_version_via_slugs


class BuildCommandResultInline(admin.TabularInline):
Expand Down Expand Up @@ -48,7 +56,6 @@ def version_name(self, obj):


class VersionAdmin(GuardedModelAdmin):
search_fields = ('slug', 'project__name')
Copy link
Member Author

Choose a reason for hiding this comment

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

This was repeated below

list_display = (
'slug',
'type',
Expand Down Expand Up @@ -135,5 +142,23 @@ def wipe_version(self, request, queryset):
wipe_version.short_description = 'Wipe version from ES'


@admin.register(RegexAutomationRule)
class RegexAutomationRuleAdmin(PolymorphicChildModelAdmin, admin.ModelAdmin):
base_model = RegexAutomationRule


@admin.register(VersionAutomationRule)
class VersionAutomationRuleAdmin(PolymorphicParentModelAdmin, admin.ModelAdmin):
base_model = VersionAutomationRule
list_display = (
'project',
'priority',
'match_arg',
'action',
'version_type',
)
child_models = (RegexAutomationRule,)


admin.site.register(Build, BuildAdmin)
admin.site.register(Version, VersionAdmin)
20 changes: 20 additions & 0 deletions readthedocs/builds/automation_actions.py
@@ -0,0 +1,20 @@
"""
Actions used for the automation rules.

Each function will receive the following args:

- version: The version object where the action will be applied
- match_result: The result from the match option
- action_arg: An additional argument to apply the action
Copy link
Member

Choose a reason for hiding this comment

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

As I said in another comment, I'd remove everything that it's not needed at this point: match_result and action_arg here.

If we want to include them in this PR, I think it would be good to have the use case in this PR also otherwise we are adding code that it's not useful and only complicate things.

"""


def activate_version(version, match_result, action_arg, *args, **kwargs):
version.active = True
version.save()


def set_default_version(version, match_result, action_arg, *args, **kwargs):
project = version.project
project.default_version = version.verbose_name
project.save()
53 changes: 53 additions & 0 deletions readthedocs/builds/migrations/0007_add-automation-rules.py
@@ -0,0 +1,53 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.20 on 2019-04-25 23:04
from __future__ import unicode_literals

from django.db import migrations, models
import django.db.models.deletion
import django_extensions.db.fields


class Migration(migrations.Migration):

dependencies = [
('projects', '0042_increase_env_variable_value_max_length'),
('contenttypes', '0002_remove_content_type_name'),
('builds', '0006_add_config_field'),
]

operations = [
migrations.CreateModel(
name='VersionAutomationRule',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('created', django_extensions.db.fields.CreationDateTimeField(auto_now_add=True, verbose_name='created')),
('modified', django_extensions.db.fields.ModificationDateTimeField(auto_now=True, verbose_name='modified')),
('priority', models.IntegerField(help_text='A lower number (0) means a higher priority', verbose_name='Rule priority')),
('match_arg', models.CharField(help_text='Value used for the rule to match the version', max_length=255, verbose_name='Match argument')),
('action', models.CharField(choices=[('activate-version', 'Activate version on match'), ('set-default-version', 'Set as default version on match')], max_length=32, verbose_name='Action')),
('action_arg', models.CharField(blank=True, help_text='Value used for the action to perfom an operation', max_length=255, null=True, verbose_name='Action argument')),
('version_type', models.CharField(choices=[('branch', 'Branch'), ('tag', 'Tag'), ('unknown', 'Unknown')], max_length=32, verbose_name='Version type')),
('polymorphic_ctype', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='polymorphic_builds.versionautomationrule_set+', to='contenttypes.ContentType')),
('project', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='automation_rules', to='projects.Project')),
],
options={
'ordering': ('priority', '-modified', '-created'),
'manager_inheritance_from_future': True,
},
),
migrations.CreateModel(
name='RegexAutomationRule',
fields=[
],
options={
'proxy': True,
'manager_inheritance_from_future': True,
'indexes': [],
},
bases=('builds.versionautomationrule',),
),
migrations.AlterUniqueTogether(
name='versionautomationrule',
unique_together=set([('project', 'priority')]),
),
]
119 changes: 119 additions & 0 deletions readthedocs/builds/models.py
Expand Up @@ -12,10 +12,13 @@
from django.utils import timezone
from django.utils.translation import ugettext
from django.utils.translation import ugettext_lazy as _
from django_extensions.db.models import TimeStampedModel
from guardian.shortcuts import assign
from jsonfield import JSONField
from polymorphic.models import PolymorphicModel
from taggit.managers import TaggableManager

import readthedocs.builds.automation_actions as actions
from readthedocs.config import LATEST_CONFIGURATION_VERSION
from readthedocs.core.utils import broadcast
from readthedocs.projects.constants import (
Expand Down Expand Up @@ -701,3 +704,119 @@ def run_time(self):
if self.start_time is not None and self.end_time is not None:
diff = self.end_time - self.start_time
return diff.seconds


class VersionAutomationRule(PolymorphicModel, TimeStampedModel):

"""Versions automation rules for projects."""

ACTIVATE_VERSION_ACTION = 'activate-version'
SET_DEFAULT_VERSION_ACTION = 'set-default-version'
ACTIONS = (
(ACTIVATE_VERSION_ACTION, _('Activate version on match')),
stsewd marked this conversation as resolved.
Show resolved Hide resolved
(SET_DEFAULT_VERSION_ACTION, _('Set as default version on match')),
)

project = models.ForeignKey(
Project,
related_name='automation_rules',
on_delete=models.CASCADE,
)
priority = models.IntegerField(
Copy link
Member

Choose a reason for hiding this comment

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

Are we using this somewhere?

I assume that this could be useful to decide what rule to apply first. If that's the case, maybe order is a better name for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is to know the order of the rules.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this defines which rule is run first. Not sure about order, it translates different without context?

_('Rule priority'),
help_text=_('A lower number (0) means a higher priority'),
)
match_arg = models.CharField(
_('Match argument'),
help_text=_('Value used for the rule to match the version'),
max_length=255,
)
action = models.CharField(
_('Action'),
max_length=32,
choices=ACTIONS,
)
action_arg = models.CharField(
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, but I can think of some cases where we could need more than one. But, this also could be solved by creating several rules with different action_arg.

Copy link
Member

Choose a reason for hiding this comment

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

How is this action_arg used from a user perspective and what is it useful for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Any extra option needed to execute the action. Like, I want to activate the version and set the privacy to public/private. But I can think this can be solved by creating two rules. First activate version v and a second one set version v to public/private.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say that we should keep this simple for now.

If 2 actions are needed for a specific version (match rule), two entries should be created. I'd remove the action_arg for now, and add it later if we find that we need it.

_('Action argument'),
help_text=_('Value used for the action to perfom an operation'),
max_length=255,
null=True,
blank=True,
)
version_type = models.CharField(
_('Version type'),
max_length=32,
choices=VERSION_TYPES,
)

class Meta:
unique_together = (('project', 'priority'),)
ordering = ('priority', '-modified', '-created')

def run(self, version, *args, **kwargs):
"""
Run an action if `version` matches the rule.

:type version: readthedocs.builds.models.Version
:returns: True if the action was performed
"""
if version.type == self.version_type:
match, result = self.match(version, self.match_arg)
if match:
self.apply_action(version, result)
return True
return False

def match(self, version, match_arg):
"""
Returns True and the match result if the version matches the rule.

:type version: readthedocs.builds.models.Version
:param str match_arg: Additional argument to perform the match
:returns: A tuple of (boolean, match_resul).
The result will be passed to `apply_action`.
"""
return False, None

def apply_action(self, version, match_result):
"""
Apply the action from allowed_actions.

:type version: readthedocs.builds.models.Version
:param any match_result: Additional context from the match operation
:raises: NotImplementedError if the action
isn't implemented or supported for this rule.
"""
action = self.allowed_actions.get(self.action)
if action is None:
raise NotImplementedError
action(version, match_result, self.action_arg)

def __str__(self):
class_name = self.__class__.__name__
return (
f'({self.priority}) '
f'{class_name}/{self.get_action_display()} '
f'for {self.project.slug}:{self.get_version_type_display()}'
)


class RegexAutomationRule(VersionAutomationRule):

allowed_actions = {
VersionAutomationRule.ACTIVATE_VERSION_ACTION: actions.activate_version,
VersionAutomationRule.SET_DEFAULT_VERSION_ACTION: actions.set_default_version,
}

class Meta:
proxy = True

def match(self, version, match_arg):
try:
match = re.search(
Copy link
Member

Choose a reason for hiding this comment

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

I'd say that what we want here is fullmatch; otherwise we are not really matching the user's regex.

.search produce unexpected results here:

In [2]: re.search('[0-9]', 'my-release-1')                                                                                                                                                    
Out[2]: <re.Match object; span=(11, 12), match='1'>

In [3]: re.fullmatch('[0-9]', 'my-release-1')                                                                                                                                                 

In [4]: re.fullmatch('[0-9]', '1')                                                                                                                                                            
Out[4]: <re.Match object; span=(0, 1), match='1'>

In [5]: re.fullmatch('[0-9]', '1a')                                                                                                                                                           

In [6]:  

Copy link
Member Author

Choose a reason for hiding this comment

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

For me the result of fullmatch is unexpected. In regex I always need to start with ^ if I want to match from the beginning

Copy link
Member

Choose a reason for hiding this comment

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

I think fullmatch does exactly what we want: matches the full string. Using this approach will avoid false positives when matching things that are not complete, as I showed in the examples above.

Copy link
Member

Choose a reason for hiding this comment

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

In regex I always need to start with ^ if I want to match from the beginning

You are an advanced user :)

I'm sure that many people will end up matching versions that they don't want to match.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is that this should not be exposed to general users, so we need all the power of regex internally more than exposing this to users.

match_arg, version.verbose_name
)
return bool(match), match
except Exception as e:
log.info('Error parsing regex: %s', e)
return False, None
2 changes: 1 addition & 1 deletion readthedocs/projects/models.py
Expand Up @@ -911,7 +911,7 @@ def update_stable_version(self):
"""
Returns the version that was promoted to be the new stable version.

Return ``None`` if no update was mode or if there is no version on the
Return ``None`` if no update was made or if there is no version on the
project that can be considered stable.
"""
versions = self.versions.all()
Expand Down
8 changes: 8 additions & 0 deletions readthedocs/restapi/utils.py
Expand Up @@ -2,6 +2,7 @@

"""Utility functions that are used by both views and celery tasks."""

import itertools
import logging

from rest_framework.pagination import PageNumberPagination
Expand Down Expand Up @@ -160,6 +161,13 @@ def delete_versions(project, version_data):
return set()


def run_automation_rules(project, versions_slug):
versions = project.versions.filter(slug__in=versions_slug)
rules = project.automation_rules.all()
for version, rule in itertools.product(versions, rules):
rule.run(version)


class RemoteOrganizationPagination(PageNumberPagination):
page_size = 25

Expand Down
13 changes: 12 additions & 1 deletion readthedocs/restapi/views/model_views.py
Expand Up @@ -8,7 +8,7 @@
from django.shortcuts import get_object_or_404
from django.template.loader import render_to_string
from rest_framework import decorators, permissions, status, viewsets
from rest_framework.parsers import MultiPartParser, JSONParser, FormParser
from rest_framework.parsers import JSONParser, MultiPartParser
from rest_framework.renderers import BaseRenderer, JSONRenderer
from rest_framework.response import Response

Expand Down Expand Up @@ -215,6 +215,17 @@ def sync_versions(self, request, **kwargs): # noqa: D205
status=status.HTTP_400_BAD_REQUEST,
)

try:
api_utils.run_automation_rules(project, added_versions)
stsewd marked this conversation as resolved.
Show resolved Hide resolved
except Exception:
# Don't interrupt the request if something goes wrong
# in the automation rules.
log.exception(
'Failed to execute automation rules for [%s]: %s',
project.slug, added_versions
)

# TODO: move this to an automation rule
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 wanted to move this to the run_automation_rules function, but it depends on the previous state of one version. It can be refactored for later

promoted_version = project.update_stable_version()
new_stable = project.get_stable_version()
if promoted_version and new_stable and new_stable.active:
Expand Down