Unfortunately scores are not additive right now. This is a great idea
feature-wise, and it may be able to be done with the Prolog
interpreter in 2.2.2 using a rules.pl inside of the project
repository. Unfortunately nobody has tried writing such a rule yet.
:-)
I think that if you made the CR category only range up to
+1, then you could remove submit from all users except for a
special hook user, and then make the comment-added hook
perform the submit when it sees the second +1.
-Martin
--
Employee of Qualcomm Innovation Center, Inc. which is a
member of Code Aurora Forum
I've done some experimentation with the prolog rules in gerrit today
and one of the things I implemented was the 1+1 = 2 rule.
Here is some notes from the session + rules for the 1+1=2.
Gerrit 2.2 Prolog rules
-----------------------
First off, this is the result of a couple of hours of experimenting,
so expect potentially incorrect information.
Shawn, please correct me if I'm making some invalid assumptions.
Introduction:
Gerrit 2.2 now supports special submit rules written in prolog. You
can enable it for a project by checking out the refs/meta/config
branch and pushing a file called 'rules.pl'.
git clone <your project>
git fetch origin refs/meta/config
git checkout -b config FETCH_HEAD
git add rules.pl ; git commit...
git push origin config:refs/meta/config
===Basics===
Each project can have their own rules.pl together with a submit_rule
predicate. It seems you can also have a submit_filter predicate that
takes in the previously set results and modify them, ie, in
all-projects or something like that, but I haven't tested that.
The submit_rule takes one 'argument' and you need to fill it in with
the form submit(label(name,status),label(name,status),...)
You can name the labels anything you want, but if your users should be
able to score them, they need to be created in the database like the
old review categories. (Anyone know if that is correct?)
I haven't found a way to set a review-score through the prolog
scripts, nor a good way to communicate to users, but I guess it's not
really intended for that.
So what can you do? Lots of data is exposed: commit message, all the
files that have been modified, author, etc etc. This is pretty well
documented in the 2.2.2 release notes.
*Warning*: Once you create a rules.pl file, the old legacy rules will
not be used!
== Example 1+1 = 2 ==
The rule allows submit if the accumulated score of the review is at
least 2. Ie, one +2 or two +1's.
%% rules.pl.
%% Allow submit if there is a total of +2 in review 'scores' (accumulative)
%% This file is an example to learn about prolog submit checkers
%%
:- package user.
%% Result returned from submit_label should be something like
%% submit(label(name,status),label(name,status),...)
%% Status can be either ok(_), reject(_), need(_) or impossible(_)
%% ok() and reject() takes a user id as their argument, but I dont
know how that is used.
sum_list([], 0).
sum_list([H | Rest], Sum) :- sum_list(Rest,Tmp), Sum is H + Tmp.
check_min_label(Category, Min, P) :-
findall(X, gerrit:commit_label(label(Category,X),_),Z),
sum_list(Z, Sum),
( Sum >= Min -> P =.. [label,Category,ok(Who)]
; P =.. [label,Category,need(Min)]
).
% This is the function that will be invoked by gerrit. We call the
check_plus2 function and returns a function submit(...)
% which gerritalways expects. (See gerrit_common.pl:133)
submit_rule(P) :- check_min_label('Code-Review', 2, HasPlusTwo),
P =.. [submit, HasPlusTwo].
=== How to test ===
Upload a change for review, then keep pushing changes to rules.pl
until it does what you want. A simple re-load on the project page will
directly use the updated rules.pl
(There is a built-in interpreter, but I haven't been able to properly
load both gerrit_common.pl together with a rules.pl (Shawn?). I did
manage to test rules locally by adding
%Test submit_rule
x_can_submit(SubmitRule, S) :-
submit_rule(Tmp),
Tmp =.. [submit | Ls],
( is_all_ok(Ls) -> S = ok(Tmp), ! ; S = not_ready(Tmp) ).
is_all_ok([]).
is_all_ok([label(_, ok(__)) | Ls]) :- is_all_ok(Ls).
is_all_ok(_) :- fail.
to my rules.pl)
but that won't work once you try to use the gerrit builtin predicates.
> --
> To unsubscribe, email repo-discuss...@googlegroups.com
> More info at http://groups.google.com/group/repo-discuss?hl=en
On Sun, Feb 5, 2012 at 09:21, Johan Bj=F6rk <p...@spotify.com> wrote:
> I've done some experimentation with the prolog rules in gerrit today
> and one of the things I implemented was the 1+1 =3D 2 rule.
Thanks for posting this!
> Here is some notes from the session + rules for the 1+1=3D2.
> Gerrit 2.2 Prolog rules
> -----------------------
>
> First off, this is the result of a couple of hours of experimenting,
> so expect potentially incorrect information.
> Shawn, please correct me if I'm making some invalid assumptions.
>
> Introduction:
> Gerrit 2.2 now supports special submit rules written in prolog. You
> can enable it for a project by checking out the refs/meta/config
> branch and pushing a file called 'rules.pl'.
>
> =A0git clone <your project>
> =A0git fetch origin refs/meta/config
> =A0git checkout -b config FETCH_HEAD
> =A0git add rules.pl ; git commit...
> =A0git push origin config:refs/meta/config
This is awkward to test with, as you note below. We should try to
improve this with a way to run these rules locally in a test
environment. Jason Tsay (my intern last summer that did a lot of this
work) suggested doing this, but had to return to university before he
could get to this. I proposed it wasn't super critical, because
administrators could set up a small H2 based test server locally to
kick around the rules on before pushing them to their production
server. Despite having that, it would be good to have a faster way to
test these.
More complex rules may also want a unit test framework.
> =3D=3D=3DBasics=3D=3D=3D
> Each project can have their own rules.pl together with a submit_rule
> predicate. It seems you can also have a submit_filter predicate that
> takes in the previously set results and modify them, ie, in
> all-projects or something like that, but I haven't tested that.
Yes. Parent projects (all the way up to All-Projects) can use
submit_filter to process the results of a child project and enforce
higher level policies. One example usage of this is a system wide rule
to require a release engineer to approve any change going to a frozen
release branch.
> The submit_rule takes one 'argument' and you need to fill it in with
> the form submit(label(name,status),label(name,status),...)
> You can name the labels anything you want, but if your users should be
> able to score them, they need to be created in the database like the
> old review categories. (Anyone know if that is correct?)
Correct. The range of potential values must come from the
approval_category_values table in the database, which means you must
also declare the label with a row in the approval_categories table.
Since the default submit rules require all of the declared
approval_categories to be satisfied to submit a change, creating a new
optional category for the Prolog rules to use in a project is awkward.
You also have to create a submit_filter in All-Projects that makes
this optional.
> I haven't found a way to set a review-score through the prolog
> scripts,
You cannot set a score through Prolog. The current system is designed
to be pure functions, with no side-effects. I never anticipated
setting scores in Prolog, I assumed the Prolog logic would decide what
labels need to be set. If the logic doesn't want to require
Code-Review +2 before submit, it can just not include the Code-Review
label in the submit compound term it returns. Or as you do where with
the "sum to 2" rule, export it as ok once its happy.
> nor a good way to communicate to users, but I guess it's not
> really intended for that.
Right now the only communication with a user is the few status codes
you found to export: ok, reject, need, impossible. The first 3 are the
common results:
ok - This label/category's rule has been met. If all
label/categories are ok the change can submit.
reject - A score is blocking submission, aka "Verified -1" or
"Code-Review -2".
need - If the right user(s) set the right scores it can be submitted.
impossible - The logic knows the change cannot be submitted as-is.
Administrative intervention is probably required. This is meant for
cases where the logic requires members of "FooEng" to score
"Code-Review +2" on a change, but nobody is in group "FooEng". It is
to hint at permissions misconfigurations.
> So what can you do? Lots of data is exposed: commit message, all the
> files that have been modified, author, etc etc. This is pretty well
> documented in the 2.2.2 release notes.
>
> *Warning*: Once you create a rules.pl file, the old legacy rules will
> not be used!
This should only happen if you declare the submit_rule predicate, or
submit_filter in a parent project. Of course that is the reason to
create this file. :-)
> =3D=3D Example 1+1 =3D 2 =3D=3D
> The rule allows submit if the accumulated score of the review is at
> least 2. Ie, one +2 or two +1's.
>
>
> %% rules.pl.
> %% Allow submit if there is a total of +2 in review 'scores' (accumulativ=
e)
> %% This file is an example to learn about prolog submit checkers
> %%
> :- package user.
>
> %% Result returned from submit_label should be something like
> %% submit(label(name,status),label(name,status),...)
> %% Status can be either ok(_), reject(_), need(_) or impossible(_)
> %% ok() and reject() takes a user id as their argument, but I dont
> know how that is used.
I don't think the Who is actually used in the Java server. I meant to
eventually export this to the UI, especially the reject(Who) case to
say something in the UI like "Submission blocked by Dr. Who". Then the
change owner can try to work out with Dr. Who why they blocked the
change. Maybe its because they are playing the role of Dr. No for this
release and have said "No, this change is too risky for this release".
:-)
> sum_list([], 0).
> sum_list([H | Rest], Sum) :- sum_list(Rest,Tmp), Sum is H + Tmp.
>
> check_min_label(Category, Min, =A0P) :-
> findall(X, gerrit:commit_label(label(Category,X),_),Z),
> sum_list(Z, Sum),
> ( Sum >=3D Min -> P =3D.. [label,Category,ok(Who)]
> =A0; P =3D.. [label,Category,need(Min)]
> =A0).
This is a bit awkward to use =3D.., can't you say:
( Sum >=3D Min -> P =3D label(Category, ok(Who))
; P =3D label(Category, need(Min))
).
I usually prefer to write these sorts of rules with a couple of cases
rather than using ->;
check_min_label(Category, Min, P) :-
findall(X, gerrit:commit_label(label(Category, X), _), List),
sum_list(List, Sum),
Sum >=3D Min,
P =3D label(Category, ok(_)).
check_min_label(Category, Min, P) :-
P =3D label(Category, need(Min)).
> % This is the function that will be invoked by gerrit. We call the
> check_plus2 function and returns a function submit(...)
> % which gerritalways expects. (See gerrit_common.pl:133)
> submit_rule(P) :- check_min_label('Code-Review', 2, HasPlusTwo),
> P =3D.. [submit, HasPlusTwo].
Again, you can use P =3D submit(HasPlusTwo). =3D.. is mainly useful for
dynamic situations where you have a list of arguments for the compound
and you don't know when you wrote the code how many arguments you
need.
We us a compound here because you can then write your submit rule like this=
:
submit_rule(submit(CodeReview)) :-
check_min_label('Code-Review', 2, CodeReview).
submit_rule(submit(CodeReview, ManagerOverride)) :-
check_min_label('Code-Review', 1, CodeReview),
check_min_label('Manager-Override', 1, ManagerOverride).
In experimentations I found it cleaner to write in the submit rule
predicate the exact categories I wanted this rule to require/enforce,
and then use variables for each of them in the body of the rule. The
two cases above say you can submit a change if you have 1+1=3D2 for
Code-Review, OR if you have 1 Code-Review and 1 Manager-Override. This
prevents 2 Manager-Overrides from making the change submittable. OK,
maybe this is a contrived example since the manager could be giving
Code-Review... but perhaps it is worthwhile to declare this exemption
as its own category.
Another might be to require ThirdParty review on files in third_party/
directory:
submit_rule(submit(CodeReview, ThirdParty)) :-
gerrit:commit_delta('^third_party/.*'),
!,
check_min_label('Code-Review', 2, CodeReview),
check_min_label('ThirdParty-Review', 1, ThirdParty).
submit_rule(submit(CodeReview)) :-
check_min_label('Code-Review', 2, CodeReview).
The ! is necessary in that first rule to cut off the other possible
ways to complete the change. Without it the server can backtrack and
choose the second case, permitting submission without the
ThirdParty-Review flag even if there is a path touched under
third_party/.
This is the real power of using a Prolog interpreter for the rule
evaluation, but its also the risk... it will backtrack. :-)
> =3D=3D=3D How to test =3D=3D=3D
> Upload a change for review, then keep pushing changes to rules.pl
> until it does what you want. A simple re-load on the project page will
> directly use the updated rules.pl
>
> (There is a built-in interpreter, but I haven't been able to properly
> load both gerrit_common.pl together with a rules.pl (Shawn?).
Not supported at this time. The problem is gerrit_common.pl requires a
huge amount of the server environment to be loaded and running,
complete with database and Git repository and an active user. I
mentioned above we would like to extend this to be a test environment
that is just like the real server, but it was difficult to setup and
my intern ran out of time during his internship to finish that work.
It might be possible to setup a mock environment using an in-memory H2
database and some in-memory Git repositories, but then you have to
worry about setting up the test change data too. What data you want in
the test change(s) and how the projects are put together in a parent
project hierarchy is difficult. So we punted. I told Jason most server
administrators keep test servers that have restored backups of their
organizations real servers (OK, I at least did that with rsac and the
internal Google server!), so any testing would be done on one of
those.
I was trying to figure this out earlier and it just doesn't seem
possible at this point.
Thanks,
Trevor
I suppose that it could be dug out of the SQL but that's a bit heavyweight.
Any pointers on how to actually find this information via gerrit (if
possible) would be appreciated.
Thanks,
Trevor
> --
> To unsubscribe, email repo-discuss...@googlegroups.com
> More info at http://groups.google.com/group/repo-discuss?hl=en
--
Trevor Vaughan
Vice President, Onyx Point, Inc
(410) 541-6699
tvau...@onyxpoint.com
-- This account not approved for unencrypted proprietary information --
> Gerrit Code Review 2.2.2
>
> 1> 'push origin config:refs/meta/config ' get feedback 'error: src
> refspec config does not match any.'
Did you have a ref named "config" in your workspace?
> 2> 'push origin config:refs/meta/config' get feedback ' ! [remote
> rejected] HEAD -> refs/meta/config (can not update the reference as a
> fast forward)
> error: failed to push some refs to ...'
Was your commit based on the tip of refs/meta/config? If not, this error
message is expected.
> 3>'push origin config:refs/heads/refs/meta/config' success however
> new branch created '* [new branch] HEAD -> refs/for/refs/meta/config'
Um, this isn't consistent. You're saying you pushed to
refs/heads/refs/meta/config, yet the output from Git talks
about refs/for/refs/meta/config. Copy/paste problem? Pushing
to refs/heads/refs/meta/config will indeed create a new branch
under refs/heads named refs/meta/config.
--
Magnus Bäck
ba...@google.com
On Thursday, March 15, 2012 at 05:42 EDT,
Bruce Zu <zu.bruc...@gmail.com> wrote:> Gerrit Code Review 2.2.2
>
> 1> 'push origin config:refs/meta/config ' get feedback 'error: src
> refspec config does not match any.'Did you have a ref named "config" in your workspace?
Introduction:
Gerrit 2.2 now supports special submit rules written in prolog. You
can enable it for a project by checking out the refs/meta/config
branch and pushing a file called 'rules.pl'.
git clone <your project>
git fetch origin refs/meta/config
git checkout -b config FETCH_HEAD
git add rules.pl ; git commit...
git push origin config:refs/meta/config
> 2> 'push origin HEAD:refs/meta/config' get feedback ' ! [remote
> rejected] HEAD -> refs/meta/config (can not update the reference as a
> fast forward)
> error: failed to push some refs to ...'
Was your commit based on the tip of refs/meta/config? If not, this error
message is expected.
> 3>'push origin config:refs/heads/refs/meta/config' success however
> new branch created '* [new branch] HEAD -> refs/for/refs/meta/config'
Um, this isn't consistent. You're saying you pushed to
refs/heads/refs/meta/config, yet the output from Git talks
about refs/for/refs/meta/config. Copy/paste problem? Pushing
to refs/heads/refs/meta/config will indeed create a new branch
under refs/heads named refs/meta/config.--
Magnus Bäck
ba...@google.com
# Checkout your meta/config branch from gerrit
git fetch origin refs/meta/config:refs/remotes/origin/meta/config
git checkout meta/config
# Make changes to your project.config file ..
git config --file project.config --add .....
# Send them back to gerrit
git commit -a project.config -m "Modified project config file"
git push origin HEAD:refs/config
--
Hi Mohamed
Thanks for your help
I test on Gerrit Code Review (2.3-rc0-67-g471e29f)
Prepare permission as
Commands in local as below
$ git clone ssh://bruce.zu@localhost:29418/ptest
Cloning into ptest...
remote: Counting objects: 2, done
remote: Finding sources: 100% (2/2)
remote: Total 2 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (2/2), done.
$:~/gerrit-code/google-gerrit/local$ cd ptest/
$ git fetch origin refs/meta/config:refs/remotes/origin/meta/config
remote: Counting objects: 13, done
remote: Finding sources: 100% (13/13)
remote: Total 13 (delta 0), reused 1 (delta 0)
Unpacking objects: 100% (13/13), done.
From ssh://localhost:29418/ptest
* [new branch] refs/meta/config -> origin/meta/config
$ git checkout meta/config
Branch meta/config set up to track remote branch meta/config from origin by rebasing.
Switched to a new branch 'meta/config'
$ git config --file project.config -e
$ more project.config
[project]
state = active
description = "test config just change here"
[access "refs/heads/*"]
owner = group Administrators
[access "refs/*"]
owner = group Administrators
create = group Administrators
push = group Administrators
$ git diff
diff --git i/project.config w/project.config
index 8028b0c..568902e 100644
--- i/project.config
+++ w/project.config
@@ -1,6 +1,6 @@
[project]
state = active
- description = "test config"
+ description = "test config just change here"
[access "refs/heads/*"]
owner = group Administrators
[access "refs/*"]
$ git commit -a project.config -m "test updage config manually "
fatal: Paths with -a does not make sense.
$ git commit -a -m "test updage config manually "
[meta/config f7628d0] test updage config manually
1 files changed, 1 insertions(+), 1 deletions(-)
$ git push origin HEAD:refs/config
Counting objects: 5, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 341 bytes, done.
Total 3 (delta 1), reused 0 (delta 0)
remote: Resolving deltas: 100% (1/1)
remote: Updating changes:done
To ssh://bruce.zu@localhost:29418/ptest
! [remote rejected] HEAD -> refs/config (Ref Ref[refs/config=4665fe5efed06e27c73c2d7344020a80f887b140] already exists)
error: failed to push some refs to 'ssh://bruce.zu@localhost:29418/ptest'
$
Bug?
/Bruce
Done by little Shift as below
$ git push origin HEAD:refs/meta/config
Counting objects: 5, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 341 bytes, done.
Total 3 (delta 1), reused 0 (delta 0)
remote: Resolving deltas: 100% (1/1)
remote: Updating changes: refs: 100% (1/1), done
To ssh://bruce.zu@localhost:29418/ptest
5747d43..f7628d0 HEAD -> refs/meta/config
$ cd ../../review/git/ptest.git/
$ more refs/meta/config |xargs git show
commit f7628d0933cf0c08ec2e58802ef2ed7f623975b0
Author: Bruce Zu <bruc...@sonyericsson.com>
Date: Fri Mar 16 17:02:45 2012 +0800
test updage config manually
diff --git a/project.config b/project.config
index 8028b0c..568902e 100644
--- a/project.config
+++ b/project.config
@@ -1,6 +1,6 @@
[project]
state = active
- description = "test config"
+ description = "test config just change here"
[access "refs/heads/*"]
owner = group Administrators
[access "refs/*"]
$
Anyway it is work on Gerrit Code Review (2.3-rc0-67-g471e29f) now
I will test 2.2.2 again when I got time.
Thanks a lot !
Hey Bruce,
Oh and don't forget the groups file that you have to create that maps uuids and group names. That should be next to the project.config file. Couple of days ago there was a thread that we should how it is structured.
Thanks,
Mohamed Mansour
On Friday, March 16, 2012, Zu, Bruce <Bruc...@sonymobile.com<mailto:Bruc...@sonymobile.com>> wrote:
> Done by little Shift as below
>
> $ git push origin HEAD:refs/meta/config
>
> Counting objects: 5, done.
>
> Delta compression using up to 4 threads.
>
> Compressing objects: 100% (3/3), done.
>
> Writing objects: 100% (3/3), 341 bytes, done.
>
> Total 3 (delta 1), reused 0 (delta 0)
>
> remote: Resolving deltas: 100% (1/1)
>
> remote: Updating changes: refs: 100% (1/1), done
>
> To ssh://bruce.zu@localhost:29418/ptest
>
> 5747d43..f7628d0 HEAD -> refs/meta/config
>
> $ cd ../../review/git/ptest.git/
>
> $ more refs/meta/config |xargs git show
>
> commit f7628d0933cf0c08ec2e58802ef2ed7f623975b0
>
> Author: Bruce Zu <bruc...@sonyericsson.com<mailto:bruc...@sonyericsson.com>>
> To unsubscribe, email repo-discuss...@googlegroups.com<mailto:repo-discuss%2Bunsu...@googlegroups.com>