On Wed, Aug 12, 2009 at 6:51 PM, Christian Hammond<chip...@chipx86.com> wrote:
> 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")

This was my suggestion of plain Python boolean expressions. It could
be just and's, or's not's, everything else being boolean functions
(your example would actually be:

greater_than(shipit_count, 3) AND in(reviewers.group, ["senior-engineering"])

Extensions can then register their own checks as additional boolean
functions, as you mentioned below.

> 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.

Not sure if nesting is necessary. Isn't any propositional logic
expression writable as a conjunction of disjunctions (with negations)?

> 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
> 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.

I was thinking about this myself. The same ExpressionBuilder (django
plugin?) could be used to create arbitrary notification subscriptions.
We could choose which signals are to be exposed, and the user could
build forwarding filters (much like Gmail):

ON review_request_published, IF (expression), POST/email to: url/email address.

I don't think this should be tied to policy though. IF (expression),
ALLOW/DENY foo is a different thing. The two can of course be composed
to achieve some workflow:

Notification filter: On (event), IF (expression), POST to:

The custom extension then would install an endpoint (URL) for a
webhook dispatched by the buildbot. Upon receipt, it would set the
"build successful" flag for the review request. The policy maker then
could build the expression:

IF (foo AND bar AND build_successful(review_request)), ALLOW: shipit.

> 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 
For more options, visit this group at 

Reply via email to