Hi Eduardo,
My biggest concern at this point is adding another dependency, especially
one that isn't pure-Python and easy_install'able. We have a lot of
dependencies today, some optional, some required. Those that are required
and pure Python are easy. We can just add them to Review Board's dependency
list. However, those that require some C components or complex installation
(such as PyLucene) can be very difficult depending on platform, distro, etc.
I wonder if we truly need that level of complexity for the implementation of
this. Having the predicate editor is good, but I don't think we need any
sort of complex DSL backing it necessarily.
Here's what I sort of envisioned:
1) Have a format stored in the database to represent the predicates. We
could either do something like JSON:
{'name': 'My Predicate',
'conditions': (
{'op': 'AND', 'conditions': (
{'op': 'GT',
'field': 'shipit_count',
'value': 3},
{'op': 'IN',
'field': 'reviewers.group',
'value': 'senior-engineering'},
})
}
... or something simpler, like:
(shipit_count > 3 AND reviewers.group IN "senior-engineering")
I prefer the former if the user will never see it, or the latter if they
will. One could argue that a predicate editor should always be available but
never required, if the user wanted to do more complex expressions involving
nested parens and such.
2) Once we have the above, which would work for many standard cases, we
could implement more custom behavior by way of custom Python code. This
would be after extensions comes in, probably, but what I envisioned was to
have an extension register new, custom checks that could be referenced along
with or instead of an expression above. This would allow for even more
custom behavior than what a DSL would allow for.
For example, imagine a company wants to have a policy that you can only
commit a review request if you have 3 ship-its *and* the branch isn't
locked, *and* the change is committed. You might have an extension that does
something like:
class MyPolicyExtension(Extension):
def __init__(self):
registerPolicyOp("BRANCH_LOCKED", self.__isBranchLocked)
registerPolicyOp("CHANGE_COMMITTED", self.__isChangeCommitted)
def __isBranchLocked(self, review_request, values):
# Do some check here to determine if values[0] (branch_name) is
locked or not, and return a bool.
def __isChangeCommitted(self, review_request, values):
# Do some check here to determine if the change represented by
review_request is committed
# or not, and return a bool.
Then you could modify your expression to be:
(shipit_count > 3 AND NOT BRANCH_LOCKED(review_request.branch) AND NOT
CHANGE_COMMITTED)
That's just a rough idea of how it could look. The idea, though, is that
you'd have some nice, simple, storable, easy to parse format (whether
something like the expression above or JSON) that represented these
expressions that, by default, knew about field names and basic operators,
and then you could extend that with custom policy extensions, which any
extension out there could register. These custom ops would appear in the
predicate editor and everything.
This gets really powerful. Imagine a BuildbotExtension that allows for
automatically posting a change to a build infrastructure, and registering a
policy operator allowing the admin to prevent changes from being marked
submitted if the build failed.
Nice thing about all this is that we can do it with custom code only,
without adding any more dependencies. It can be completely under our
control.
Some food for thought :)
Christian
--
Christian Hammond - chip...@chipx86.com
Review Board - http://www.review-board.org
VMware, Inc. - http://www.vmware.com
On Tue, Aug 11, 2009 at 2:01 PM, Eduardo Felipe
<eduardofelip...@gmail.com>wrote:
> Hi there!
>
> I'm Eduardo and I'm working on Reviewboard for Google Summer of Code.
> My proposal included policy support, and as I was discussing it with
> Christian on IRC when he asked me to open up the discussion to the
> entire community.
>
> So, to quote (parts of) my proposal:
>
> <quote>
>
> During the deployment of ReviewBoard in my last employer we've had to
> establish that no code is good until it was reviewed by at least three
> programmers, two being senior. Since currently there is no way to
> specify this in ReviewBoard reviews sometimes ended Close as submitted
> without the minimum reviews rule. Talking to everyone about it solved
> the problem, but in a large organization there should be a way to
> prevent users from breaking the rules.
>
> As such I propose, based on the suggestion on the Wiki, to create an
> admin module to allow arbitrary policies for common actions. In this
> way a rule could be created for anything, from reviewing, to updating
> a diff, to marking it "Ship it!" or allowing it to be closed, deleted
> or submitted, viewing a review request, reviewing it, joining a group,
> etc. covering all the aspects needed for policy support.
>
> A suggested implementation would be based on filters, AKA predicates,
> used to allow or disallow an specific action.
>
> The priority should be what users want the most, and the interface can
> be done in the regular way or providing a Domain Specific Language, a
> very reduced subset of python, much similar to Django's template
> engine.
>
> Optionally the access to repository could be based on user's
> permission, instead of a global repository permission, by using the
> user's own credential to the repository, adding an extra level of
> protection.
>
> </quote>
>
> Now, to get to the good stuff:
>
> I think it is necessary to have a way of defining arbitrary rules
> based on the attributes of objects involved in an action.
> For instance, the objects involved in the action of "Close as
> submitted" is the user who is performing the action and the review he
> is performing it to.
>
> Now one could want to, for instance, create a rule such as "Review
> requests can only be closed as submitted if they have at least two
> ship-its". In order to express that in a neat way, I'm thinking about
> implementing what is called a "Predicate Editor" or "Expression
> Builder". I've attached a couple of examples so you can get a grip of
> what I'm thinking of doing.
>
> This predicate editor can test any aspect of pretty much any attribute
> of the objects involved in the action. So you can check things like
> "User belongs to group Foo" or even the negation as "User does not
> belong to group Foo", strings attributes such as "Review title
> contains BAR", date attributes "Review updated in the last 3 days",
> etc.
>
> By now you can imagine this is a complex feature. Indeed is so complex
> that having a Domain Specific Language for dealing with those
> predicates is NOT an overkill. The idea behind having a DSL is that we
> can store the predicates as text in the database, allowing them to be
> shared and migrate, but this DSL is not Python code, nor it should be,
> and as such it needs to be compiled down to python, evaluated and
> stored into memory, so they can be used to test actions later on.
>
> There is a limit of how far a regex based parser can go. If the DSL
> turns out to be a Context free language, regex simply won't be able to
> parse it. Now we can have two approaches for this problem:
> 1 - Create the DSL as an XML-type language, and use a XML parser + a
> custom DOM walker to generate the python code.
> 2 - Create a DSL thats more human friendly than XML, and use a
> compiler generator tool, like ANTRL, to generate a compiler that will
> in turn yield the python code.
>
> The advantage of approach 1 is that it won't need any external
> library, as python already have XML parsing libs included. The
> disadvantage of this approach is that I think it will be harder to
> build a parser for the web interface that also manipulates it. I could
> be wrong, but I've been burned before by XML in a browser, and I
> started using JSON, which is more "javascript friendly".
>
> Using the second approach is easier to build a small, readable,
> parseable language, but in order to compile it down to python we would
> need to include an external resource, such as a runtime lib for ANTRL,
> and that could have a great onus for the developers (the install of
> ANTLR is kinda of a PITA).
>
> This is a hard decision, so I would very much appreciate you input on
> it. If there is a policy wanted that a predicate engine would not be
> able to check, please let me know so we can figure something else out.
>
> Thanks a lot.
>
> Eduardo.
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"reviewboard" group.
To post to this group, send email to reviewboard@googlegroups.com
To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
-~----------~----~----~----~------~----~------~--~---
