-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes from all commits
2ae80d1
d0e04e0
d53bad2
d287286
c5e6010
cb5e1b9
4fd56f6
e570a58
353bc8a
7a60668
409e960
8f58e77
8dfe290
0d4ab95
74f1b9a
7936f55
4c77043
11add5d
5e90f62
0903763
26fc59b
7660b86
a2c50ac
0f06958
bf2f954
73030c0
6237ca4
25faa28
eda1cd2
39c8cc5
a6d959d
2ae831d
78e6e62
5d0c706
b2a1339
b07b660
a86d427
cbce26a
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 |
---|---|---|
@@ -0,0 +1,38 @@ | ||
""" | ||
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 | ||
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. As I said in another comment, I'd remove everything that it's not needed at this point: 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. |
||
""" | ||
|
||
from readthedocs.core.utils import trigger_build | ||
|
||
|
||
def activate_version(version, match_result, action_arg, *args, **kwargs): | ||
""" | ||
Sets version as active. | ||
|
||
It triggers a build if the version isn't built. | ||
""" | ||
version.active = True | ||
version.save() | ||
if not version.built: | ||
trigger_build( | ||
project=version.project, | ||
version=version | ||
) | ||
|
||
|
||
def set_default_version(version, match_result, action_arg, *args, **kwargs): | ||
""" | ||
Sets version as the project's default version. | ||
|
||
The version is activated first. | ||
""" | ||
activate_version(version, match_result, action_arg) | ||
project = version.project | ||
project.default_version = version.slug | ||
project.save() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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')]), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ( | ||
|
@@ -736,3 +739,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( | ||
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. 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 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. Yeah, this is to know the order of the rules. 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. 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( | ||
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. 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. 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. How is this 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. 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 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'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 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( | ||
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'd say that what we want here is
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. For me the result of fullmatch is unexpected. In regex I always need to start with 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 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.
You are an advanced user :) I'm sure that many people will end up matching versions that they don't want to match. 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. 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 |
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 was repeated below