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

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Feb 20, 2019

Still wip, I would like some input. I want to use the same pattern that we use for subclassing Integrations. Also, not sure if the model is fine in the builds app or if I should create a new app. I'm subclassing regex as a simple example for now.

Ref #4001

@stsewd stsewd marked this pull request as ready for review Feb 20, 2019
max_length=32,
choices=RULE_TYPES,
)
rule_arg = models.CharField(
Copy link
Member Author

@stsewd stsewd Feb 20, 2019

Should this be rule_args? I can only think of one arg for the rule.

max_length=32,
choices=ACTIONS,
)
action_arg = models.CharField(
Copy link
Member Author

@stsewd stsewd Feb 20, 2019

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

@humitos humitos Mar 4, 2019

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

Copy link
Member Author

@stsewd stsewd Mar 4, 2019

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

@humitos humitos Mar 5, 2019

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.

return self.apply_action(version, match_result, self.action_arg)
return False

def match(self, version, arg):
Copy link
Member Author

@stsewd stsewd Feb 20, 2019

I'm thinking is passing *args and **kwargs to match and apply_action, still not sure. And maybe select a better name for arg. Or just use self.rule_arg, but this makes it easier to test :)

Copy link
Member

@humitos humitos Mar 4, 2019

I think it could be better to use self.rule_arg inside the method because it will more clear when reading the code and it's a common pattern.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 28, 2019

django-polymorphic works great with proxy models, no extra tables :). I'm asking for an early review here.

@stsewd stsewd requested a review from Feb 28, 2019
Copy link
Member

@humitos humitos left a comment

This is good. I like the pattern designed here.

readthedocs/builds/models.py Outdated Show resolved Hide resolved
readthedocs/builds/models.py Outdated Show resolved Hide resolved
proxy = True

def match(self, version, arg):
regex = re.compile(arg)
Copy link
Member

@humitos humitos Mar 4, 2019

There is no need to compile the regex if it will be used just once. You can just call re.match directly.


def match(self, version, arg):
"""
Returns an object different from None if the version matches the rule.
Copy link
Member

@humitos humitos Mar 4, 2019

I think it's better to return True if it matches and False if there is no matching.

What is the match_result useful for?

Copy link
Member

@humitos humitos Mar 4, 2019

Actually, in case that match_result would be useful for something I'd suggest to have a specific method for that and leave match returning a boolean.

Copy link
Member Author

@stsewd stsewd Mar 4, 2019

We need to pass the match result, in case of regex, that would be used to capture the groups in a later phase of the feature.

Copy link
Member

@humitos humitos Mar 5, 2019

How exactly that "later phase of the feature" will use it?

Copy link
Member

@humitos humitos Mar 5, 2019

I mean, I don't see how the match result will be useful from a user's perspective. Or, how the user will have access to it.

In case we need it internally, how are we going to us it?

max_length=32,
choices=ACTIONS,
)
action_arg = models.CharField(
Copy link
Member

@humitos humitos Mar 4, 2019

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

readthedocs/builds/models.py Show resolved Hide resolved
return self.apply_action(version, match_result, self.action_arg)
return False

def match(self, version, arg):
Copy link
Member

@humitos humitos Mar 4, 2019

I think it could be better to use self.rule_arg inside the method because it will more clear when reading the code and it's a common pattern.

@@ -0,0 +1,2 @@
def activate_version_from_regex_action(version, match_result, arg):
Copy link
Member

@humitos humitos Mar 4, 2019

Is it needed to be specific here on from_regex_action?

I suppose that this function should be just like version.active = True and version.save(). No need to use match_result nor arg here.

Copy link
Member Author

@stsewd stsewd Mar 4, 2019

All function actions will receive all the parameters. Match result and arg was explained above ^

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 4, 2019

For the moment I'm going to assume that having separate rules is the way to go instead of having extra parameters (only for the names, models will still have that till final decision).

/cc @agjohnson

@@ -48,7 +56,6 @@ def version_name(self, obj):


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

@stsewd stsewd Mar 4, 2019

This was repeated below

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 4, 2019

This is ready for review, consider #5323 (comment)

This is how the admin looks like, we can expand it with search and other stuff as we need them.

screenshot_2019-03-04 select version automation rule to change django site admin

@stsewd stsewd requested review from agjohnson and Mar 4, 2019
@humitos
Copy link
Member

@humitos humitos commented Mar 5, 2019

For the moment I'm going to assume that having separate rules is the way to go instead of having extra parameters (only for the names, models will still have that till final decision).

I agree with this.

Also, it may worth to find other services that allow these kind of rules and see how they handle this case. Docker Hub for example, does something simple where you write a regex rule to be tagged as a Docker tag, like ^releases/3\.[0-9]$ will be tagged as 3.x. That's all.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 26, 2019

It wasn't so much code after all, need to write tests though

project.slug, added_versions
)

# TODO: move this to an automation rule
Copy link
Member Author

@stsewd stsewd Apr 26, 2019

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

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 29, 2019

Test are written 📖 and as usual they helped me to fix bugs on time :)

@stsewd stsewd requested review from humitos and Apr 30, 2019
Copy link
Member

@humitos humitos left a comment

💯

I suppose that at this point, I will have to accept the usage of re.search ;)

readthedocs/restapi/views/model_views.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member

@humitos humitos commented May 9, 2019

👍

@humitos
Copy link
Member

@humitos humitos commented May 20, 2019

@stsewd I think this PR is ready to merge. Can you please resolve the conflicts so we can merge it?

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 20, 2019

Done 👍

@humitos humitos merged commit ae7c700 into readthedocs:master May 21, 2019
1 check passed
@stsewd stsewd deleted the add-models-for-automationrules branch May 21, 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.

None yet

2 participants