Change traceability
Rafael Rabelo
Ulrik Sjölin, Carlos Baldacin and I have been discussing about
"marking" a change with a property and then being able to search for
that property and get only the changes that has that property set. We
would like to know your thoughts about it.
The problem is the lack of traceability in the changes we submit to
you. In order to solve it, one idea is to associate a category with a
change. A new category would be created similar to “Verified”, and it
would be only available to be set on a change if the rights apply. The
search mechanism would be modified so the user could type something
like: “category:+1”. Initially it seems feasible to me, what do you
think? @Ulrik, does it fits 100% our need?
Please, let us know if you would have suggestions, complaints about
it, or if you have thought of a different solution to the problem we
exposed.
Shawn Pearce
I agree, this is a really useful feature. In my opinion, what
you are proposing is issue 287 [1]. What I wanted to do for that
issue was:
* Combine ApprovalCategory and ApprovalCategoryValue into Label
Take CodeReview for example. Join the values up to the category
and create a number of labels:
CodeReview+2
CodeReview+1
CodeReview-0
CodeReview-1
CodeReview-2
Single value categories like Submit can just be Submit.
Store these in some new CommonLabel entity, keyed by Project.
During transition lets put all of the existing approval categories
into -- All Projects --, with the idea being that CommonLabel is
inherited down the project hierarchy just like access is.
* Modify PatchSetApproval to be PatchSetLabel
Swap out the ApprovalCategory.Id and value for a label string.
* Modify RefRights to be Label specifications
Combine the ApprovalCategory.Id, min and max specifications
into a single string. For example:
CodeReview=-2..+2
CodeReview=-1..+1
Submit
Read+1
* Add label fields to the PublishCommentsScreen
Give users fields they can enter labels into, maybe with
auto-complete support. We can still do radio buttons for
commonly used labels that are supposed to be single valued.
* Have a label "language" derived slightly from the language used
by Google Code Project Hosting [2,3].
It seems like a lot, but I don't really feel like it is. I'm
probably way more familiar with the code base than anyone else,
and thus can do stuff pretty quickly in it, but I feel like this
is something I can pull off in about 1-2 days of effort. So its
not impossible for someone to get done.
Once we have PatchSetLabel in the database, and we can add any
labels in the PublishCommentsScreen, we can start to get all sorts of
interesting ways to tag changes, find changes that have a particular
status, etc.
Its just a matter of having a search function that can take a search
string like "-Verified CodeReview>+1 Important" to find all changes
that have not been verified yet, have at least CodeReview+1, and
were given the Important flag.
Being able to use a label to store aribtrary small bits
of information on a change would really help a lot of users.
They can really start to integrate Gerrit into their workflows.
It probably would also make the new TrackingReference table obsolete.
Instead of storing ids in the TrackingReference table we can make
them labels like "Bug-381913" or "TR-88918", and you can search
for patch sets which have those labels.
[1] http://code.google.com/p/gerrit/issues/detail?id=287
[2] http://code.google.com/p/support/wiki/IssueTracker#Labels
[3] http://code.google.com/p/gerrit/issues/searchtips
Rafael Rabelo
Thanks a lot for the answer.
I have read it, study it, but I still have questions about. Please,
could you help me with them?
1) Would you mean there should be a new CommonLabel database table to
be created? In case that is the idea, would it really be necessary
(since it seems it would have same information the RefRights one)?
2) Since the RefRights should be modified, what do you think is the
best approach: do not change the UI of the Admin->Projects->Access tab
(leaving it as it is now, by parsing the Labels supposed to be the
access ones), or changing it to let the user know the access is
defined by "special" labels? I am considering there should be a column
in the Label table to indicate the "access labels" and the position
column remains indicating the "access label" is an action one.
3) Should I consider 2 different patches (features) to deliver: one to
modify Gerrit structure to consider Label use, as you suggested, and
another one to provide an additional search mechanism to find project/
changes with specific labels?
Thanks.
Rafael.
Shawn Pearce
<rafael.ra...@sonyericsson.com> wrote:
> 1) Would you mean there should be a new CommonLabel database table to
> be created? In case that is the idea, would it really be necessary
> (since it seems it would have same information the RefRights one)?
Right, recommended labels for a given project can be obtained by
scanning the relevant RefRights. This is actually a really smart
idea, because it then permits the recommended labels to be per-project
and even per-branch, offering greater configuration flexibility.
The only reason to have a common label table is to help the project
owner fill out RefRight objects, and avoid typos here. However,
suggestions can be made based on other RefRights already in this
project, and in other projects on the same server. So a common label
table might not be so useful. For the magical labels handled
specially internally by Gerrit Code Review like "Read+1" or "Submit"
we can just hard code these in an array in some class, and union that
with the labels discovered from other RefRights.
However, a common Label class would still be useful in the Java code,
to help annotate that this thing is a label, and keep operations that
are related to label handling together, like splitting off the "-1" or
"+2" suffix, or performing a range test to see if "CodeReview+2" is
inside of the range indicated by the label expression
"CodeReview=-2..+2".
> 2) Since the RefRights should be modified, what do you think is the
> best approach: do not change the UI of the Admin->Projects->Access tab
> (leaving it as it is now, by parsing the Labels supposed to be the
> access ones), or changing it to let the user know the access is
> defined by "special" labels?
I would try to rethink that Access tab UI a bit. For range labels
like CodeReview those min/max drop downs are a pain to work with. Two
groups of radio buttons might be easier, but so would just typing out
the range. The actual meaning assigned to the -2, -1, 0, +1, +2
buckets is entirely local project convention. It might just be OK to
have a text box here and type in the string "CodeReview=-2..+2".
Certainly would simplify some of the implementation of this change.
We can always come back and improve on that UI before we release a
version with the label features.
> I am considering there should be a column
> in the Label table to indicate the "access labels" and the position
> column remains indicating the "access label" is an action one.
Hmm, right. Despite my remarks above about not needing a Label table,
we need something that tells us "these labels should appear on a patch
set", so we know what to draw into the table on the change screen, and
in what order they should appear.
The way I was thinking about doing this is probably a radical
departure from what we do now. I was considering adding some sort of
rule language to the Access panel on a project. What we really want
to write is something like:
On 'branch:refs/heads/*'
Submit only if
'CodeReview+2 Verified+1'
On 'branch:refs/heads/public'
Submit only if
'CodeReview+2 Verified+1 LegalReview+1'
On 'branch:refs/heads/master ExtraReviewRequired'
Submit only if
'CodeReview+2 Verified+1 DrNo+1'
I'm not sure how to store these rules. But the general idea would be,
we display labels on the change screen that are listed as being
necessary in order to use Submit. That set of labels can change,
depending on the project, branch, and possibly even other labels being
set on the change. So in the example above, normally DrNo isn't
required for submit... but if a reviewer adds the label
ExtraReviewRequired while reviewing the change, and its on the master
branch, then we start showing the DrNo column and an additional DrNo+1
approval is needed to submit.
This, plus the stuff mentioned earlier about discovering labels from
RefRights, means we might not need a common Label table, and the
position data isn't stored in the database anymore. Instead its in
these rules.
To actually implement the "on" filter and the "only if" condition, I
was thinking about using search strings. That is, take the
ChangeQuery stuff and start extending it to handle these conditionals.
When we need to figure out what to use, we check the change against
each of these search strings, if it matches the search string, then
the rule applies, or we can enable the submit button. This may make
the rule evaluation easier for administrators, as they can "test" a
rule by searching with it. And we'll eventually get a more powerful
search.
I haven't put a lot of thought into this, much of it just came to me
one morning, and then I "shelved" it. Its possible this is going to
be far harder than it sounds, or its possible its entirely feasible
and would clean up/remove a lot of the really crappy code we have
(like FunctionState... ick).
> 3) Should I consider 2 different patches (features) to deliver: one to
> modify Gerrit structure to consider Label use, as you suggested, and
> another one to provide an additional search mechanism to find project/
> changes with specific labels?
2 or more, yea. I hate reviewing really big patches. Smaller steps
working in a logical progression is often easier to follow. If you
want to, we can create an exp-label branch and start doing the work
there in smaller steps... and when its ready, just merge it all over
to the master branch. That may make it easier to work through this,
since it could be somewhat disruptive.
Rafael Rabelo
Thanks a lot for your answers.
Carlos Baldacin and I have discussed the ideas exposed here, initially
our idea is to provide a first patch creating a fourth Admin->Project
tab in parallel with the current Access tab so that you can compare
both easily, and as soon as we have all this discussion matured and
implemented we replace the Access one. What do you think?
Off course we can do that inside the branch you mentioned. We think
it's ok!
In this initial patch we plan to create a "Label Access" UI tab
completely functional, even creating the Label database table and a
new RefRight table (RefRightLabel table maybe), but mantaining the
ApprovalCategory, ApprovalCategoryValue, so all logic related to
RefRights.
We are considering there should be the concept of 'Actions', something
like the ApprovalCategories as you have today. The actions should be
the 'special labels' we have talked about, like, for instance:
- Submit
- NoReadAccess (Read-1)
- ReadAccess (Read+1)
- UploadPermission (Read+2)
- Own
- CodeReview
- PublishLabels
Some actions can be associated with label, as you have shown to the
Submit and CodeReview one. So this could permit the format of access
to the actions:
On 'branch:refs/heads/master group:Test'
CodeReview displaying
'CodeReview+2 Verified+1 ExtraRev'
On 'branch:refs/heads/master group:Test'
CodeReview displaying
'CodeReview+2 Verified+1 ExtraRev DrNo'
only if
'ExtraRev'
On 'branch:refs/heads/master group:Test'
Submit only if
'CodeReview+2 Verified+1 ExtraRev DrNo'
On 'group:TestGroup'
Own only if
'Exception'
On 'branch:refs/heads/* group:TestGroup'
NoReadAccess
The Access UI would then be re-imagined to something like:
+--------------------------------------------------------+
| Action | Branch | Group | Cond.Labels *| Disp.Labels **|
+--------------------------------------------------------+
| Code | refs/ | Test | | CodeReview+2 |
| Review | heads/ | | | Verified+1 |
| | master | | | ExtraRev |
+--------------------------------------------------------+
| Code | refs/ | Test | ExtraRev | CodeReview+2 |
| Review | heads/ | | | Verified+1 |
| | master | | | ExtraRev |
| | | | | DrNo |
+--------------------------------------------------------+
| Submit | refs/ | Test | ExtraRev | |
| | heads/ | | CodeReview+2 | |
| | master | | Verified+1 | |
| | | | DrNo | |
+--------------------------------------------------------+
| Own | refs/* | Test | | |
| | | | | |
| | | | | |
+--------------------------------------------------------+
*Labels which must be already applied into the change in order to
allow the action to be executed.
**Labels which will be displayed in the change screen just after the
action request.
I am considering some actions, like the 'Own' one, would not have
labels associated with. Since there are some other situations not
allowed like Submit as "Cond. Label" of Code Review action, we plan to
map these rules in the database.
What do you think about the proposal? We think this is a good start.
Thank you very much!
BR
Rafael.
On Jun 23, 4:58 pm, Shawn Pearce <s...@google.com> wrote:
> On Wed, Jun 23, 2010 at 08:03, Rafael Rabelo
>
Shawn Pearce
<rafael.ra...@sonyericsson.com> wrote:
> Carlos Baldacin and I have discussed the ideas exposed here, initially
> our idea is to provide a first patch creating a fourth Admin->Project
> tab in parallel with the current Access tab so that you can compare
> both easily, and as soon as we have all this discussion matured and
> implemented we replace the Access one. What do you think?
OK.
> Off course we can do that inside the branch you mentioned.
I just created exp-label, forked from current master. We can do this
work there.
> In this initial patch we plan to create a "Label Access" UI tab
> completely functional, even creating the Label database table and a
> new RefRight table (RefRightLabel table maybe), but mantaining the
> ApprovalCategory, ApprovalCategoryValue, so all logic related to
> RefRights.
OK. That seems like a good start.
> We are considering there should be the concept of 'Actions', something
> like the ApprovalCategories as you have today. The actions should be
> the 'special labels' we have talked about, like, for instance:
> - Submit
> - NoReadAccess (Read-1)
> - ReadAccess (Read+1)
> - UploadPermission (Read+2)
> - Own
> - CodeReview
> - PublishLabels
I like this.
CodeReview isn't special, its conceptually a user created label.
Though after reading further, it seems you have some idea of this
being special. Can you elaborate a little bit on that?
What does PublishLabels mean?
I'm not sure a table is going to be the best way to layout this data,
but lets give it a try anyway.
> I am considering some actions, like the 'Own' one, would not have
> labels associated with.
Yes.
> Since there are some other situations not
> allowed like Submit as "Cond. Label" of Code Review action, we plan to
> map these rules in the database.
Good point.
> What do you think about the proposal? We think this is a good start.
I do too. If we can get this done, it will bring a lot of
functionality to folks.
Rafael Rabelo
> CodeReview isn't special, its conceptually a user created label.
> Though after reading further, it seems you have some idea of this
> being special. Can you elaborate a little bit on that?
assuming a change may be commented only by specific groups on specific
target branches.
The displayed labels in the publish comments screen will be the
‘display labels’ associated to the CodeReview action, forming the rule
which says:
On action Code Review show all labels previously set as ‘display
labels’.
A rule could also be created from conditional labels associated with a
change.
Imagine you click on Review button on a change and the ExtraRev label
is displayed because, as already exposed, the admin user set
previously this label as ‘display label’ for your group and branch.
Ok, then, you check the ExtraRev label checkbox and finally click on
Publish comments button.
It means that you have “tagged” the change with that label.
Another rule may also be created which says for instance:
Performs action Submit only if tagged with ExtraRev and CodeReview+2
Or even:
Performs action CodeReview only if tagged with ExtraRev and CodeReview
+1 (if you didn’t check ExtraRev, Code Review becomes unavailable)
> What does PublishLabels mean?
Please, ignore the PublishLabels one, my mistake.
Thanks a lot for your comments.
BR
Rafael.
Shawn Pearce
<rafael.ra...@sonyericsson.com> wrote:
>> CodeReview isn't special, its conceptually a user created label.
>> Though after reading further, it seems you have some idea of this
>> being special. Can you elaborate a little bit on that?
>
> The CodeReview action would be the user capability to review a change,
> assuming a change may be commented only by specific groups on specific
> target branches.
Ah, OK. So this is a new permission being introduced that controls
whether or not you can even review a given change? Is this something
you actually need? I prefer to take the more open approach, which is
to allow anyone to review any change they can read. However, how they
can update the labels on the change may be restricted given their
group, and the labels that are already set on it.
> The displayed labels in the publish comments screen will be the
> ‘display labels’ associated to the CodeReview action, forming the rule
> which says:
> On action Code Review show all labels previously set as ‘display
> labels’.
This sounds unnecessarily complex. The set of labels to display
should be any label that one of your groups has access to apply to
this change. If the administrator granted you access to use the
label, that should be sufficient to cause us to show it to you when
you click "Review".
Rafael Rabelo
> >> CodeReview isn't special, its conceptually a user created label.
> >> Though after reading further, it seems you have some idea of this
> >> being special. Can you elaborate a little bit on that?
>
> > The CodeReview action would be the user capability to review a change,
> > assuming a change may be commented only by specific groups on specific
> > target branches.
>
> Ah, OK. So this is a new permission being introduced that controls
> whether or not you can even review a given change? Is this something
> you actually need? I prefer to take the more open approach, which is
> to allow anyone to review any change they can read. However, how they
> can update the labels on the change may be restricted given their
> group, and the labels that are already set on it.
below table to indicate the labels to display in publish comments
screen by now, when defining an evolution of the UI, what do you
think?
+--------------------------------------------------------+
| Action | Branch | Group | Cond.Labels *| Disp.Labels **|
+--------------------------------------------------------+
| Code | refs/ | Test | | CodeReview+2 |
| Review | heads/ | | | Verified+1 |
| | master | | | ExtraRev |
+--------------------------------------------------------+
>
> > ‘display labels’ associated to the CodeReview action, forming the rule
> > which says:
> > On action Code Review show all labels previously set as ‘display
> > labels’.
>
> This sounds unnecessarily complex. The set of labels to display
> should be any label that one of your groups has access to apply to
> this change. If the administrator granted you access to use the
> label, that should be sufficient to cause us to show it to you when
> you click "Review".
I was thinking of defining if an user would have the permission of
creating branches when uploading changes by something like:
+--------------------------------------------------------+
| Action | Branch | Group | Cond.Labels *| Disp.Labels **|
+--------------------------------------------------------+
| Branch | heads/ | | | |
| | * | | | |
+--------------------------------------------------------+
instead of the way we have today, using approval categories.
Please, let me know if my understanding is correct, or if it is too
much complex. We will start developing a first patch as mentioned in
previous messagens.
BR.
Rafael.
cbaldacin
is how to consider CodeReview.
My understanding from now on is that we will not consider CodeReview
as an action like Submit is.
CodeReview will no longer be an ApprovalCategory as well. Instead, it
will be a Label available to tag a change according to the rights
previously set for a group.
For instance, group A may apply labels blah, CodeReview+1, Verified.
Since this is right, I think we have enough information to start it
up.
BR
Carlos
On 28 jun, 15:17, Rafael Rabelo <rafael.rabelosi...@sonyericsson.com>
wrote:
Shawn Pearce
<carloseduar...@sonyericsson.com> wrote:
> As far as I can tell, the only different point regarding our proposal
> is how to consider CodeReview.
You and I are on the same page here.
> My understanding from now on is that we will not consider CodeReview
> as an action like Submit is.
The CodeReview approval category isn't special in current Gerrit. Not
like submit at all.
> CodeReview will no longer be an ApprovalCategory as well. Instead, it
> will be a Label available to tag a change according to the rights
> previously set for a group.
> For instance, group A may apply labels blah, CodeReview+1, Verified.
Exactly.
> Since this is right, I think we have enough information to start it
> up.
Great!
Shawn Pearce
<rafael.ra...@sonyericsson.com> wrote:
>
> I was thinking of defining if an user would have the permission of
> creating branches when uploading changes by something like:
>
> +--------------------------------------------------------+
> | Action | Branch | Group | Cond.Labels *| Disp.Labels **|
> +--------------------------------------------------------+
> | Create | refs/ | Test | | |
> | Branch | heads/ | | | |
> | | * | | | |
> +--------------------------------------------------------+
>
> instead of the way we have today, using approval categories.
Yes, this makes sense.
Fredrik Luthander
Since we're discussing a rewrite of the rights system anyway, I have a
small request to make on this topic.
Some background:
As the number of lines in my ref_rights table rapidly approaches 5000
(47xx today), it has become apparent how difficult it is to keep track
of everything. Every so often my support group gets the question "Why
can't I [verify,code review, submit] my change?"
Now, sometimes they have a tehnical problem, but more and more often
it's a lack of rights. So, from an admins standpoint the answer that
can be given is "Because you're not part of the group that's allowed
to".
The problem is that this answer is not the most customer oriented
answer, it doesn't really include any information on what the next
step would be if the person wanted this action to be performed. So,
after a while the admins supporting gerrit learns the most common
questions, so they can tweak the answer a bit "You don't have rights
to do that, but I happen to know that Person X was the person that put
this restricting rule in place. Talk to her!" That answer is better,
it provides the next step in order to complete the change.
Suggestion:
So. Today we keep this information in our heads or in a system outside
of Gerrit. It would actually be quite handy if we could have a sort of
comment field or similar where we could have additional information
about the rule, the responsible enforcerer of the rule, or perhaps
where to turn for more information. The admins have a strictly
technical role to enforce the rules, but doesn't middle in politics
and so they need to know where to refer the support to. Would it be
possible to add another "description" field in the ref_rights table
(or the corresponding upcoming solution) or similar that would allow
for us to store some additional data in connection to all rules we
add?
Thanks,
Fredrik
Shawn Pearce
Interesting observation. Currently we don't log who added a
ref_right, or when. We should be logging all edits made to the
ref_rights table, and as part of that log we should allow you to store
an optional short note with the change made. Then you can see what
has happened, when, why.
As for telling users about why they can't do X, we need to do that
better in the UI. I've long had this dream that the access control
system should be built on top of a Prolog interpreter rather than
being hardcoded in Java, because then we can run the Prolog
interpreter to tell us not only "can I do X", but "to do X, what Y
must I have"... where Y is the approval bit that is missing, or the
group you need to be a member of, or who is a member of the group that
has the necessary flag. The idea being that a user could see on the
change page (or have a dialog pop up if its too expensive to always
compute) telling them something like:
To submit this change, you need:
Verified +1 from a Verifier. To find a Verifier, click here.
Code Review +2 from a Code Reviewer. To find a Code Reviewer, click here.
DrNo +1 from a DrNoMaster. Required on branch stable. To find a
DrNoMaster, click here.
FWIW, I've been looking at PrologCafe [1], as it is dual-licensed
under EPL/GPL, and we can safely include it under the EPL. But I
haven't dug deep enough into implementing something based on top of
it. Anyway, more pie-in-the-sky dreaming from me. I'm holding off on
the Prolog idea until after we can at least shift towards labels a
bit.
Magnus Bäck
Shawn Pearce <s...@google.com> wrote:
> Interesting observation. Currently we don't log who added a
> ref_right, or when. We should be logging all edits made to the
> ref_rights table, and as part of that log we should allow you to
> store an optional short note with the change made. Then you can
> see what has happened, when, why.
Sounds like a job for Git, doesn't it? I believe Gitosis has a
configuration git that administrators push changes into and that seems
like what we need here. It seems to fit very well with the notion of
moving Gerrit away from the database and into Git.
Now, this should perhaps not be the default option for admins -- the
situation for Fredrik and his team is probably fairly unusual. How about
keeping the existing database table but giving admins an option to make
the web interface read-only and instead accept configuration file
changes via Git? This would also open for describing permissions in a
(more) high-level fashion.
> As for telling users about why they can't do X, we need to do that
> better in the UI. I've long had this dream that the access control
> system should be built on top of a Prolog interpreter rather than
> being hardcoded in Java, because then we can run the Prolog
> interpreter to tell us not only "can I do X", but "to do X, what Y
> must I have"... where Y is the approval bit that is missing, or the
> group you need to be a member of, or who is a member of the group that
> has the necessary flag.
Okay, so I wouldn't have suggested Prolog myself but why not? Apart
from what Fredrik brought up, one thing that annoys me about the
current permission configuration is that it's too low-level, concrete,
and doesn't allow grouping of permissions.
For example, our users' default permissions don't include Push Branch +1.
Everything needs to be reviewed prior to submission. In some cases
people obtain source code from third parties directly via Git
or indirectly via git-svn and want to upload those commits to reference
branches (or upstream branches or whatever you want to call them) that
are possibly merged to our own branches. This requires the users to have
Push Branch +2 and Forge Identity +2 for the reference branch so that
the commits can be pushed as-is. I'd find it so much more elegant if it
was possible to express this as
group 'acme-import-team' is '3rd-party-importer'
for ref 'refs/heads/acme/master' in 'platform/vendor/acme'
rather than
acme-team PB +2 refs/heads/acme/master platform/vendor/acme
acme-team Fg +2 refs/heads/acme/master platform/vendor/acme
(Sorry, don't remember the correct permission group acronyms.)
This would be really convenient if a role suddenly starts to require
other permissions; you'd just change the role definition and push the
change to Gerrit, which would convert it to a database representation
and store it in the same manner as today.
I've been meaning to talk to Fredrik about something like this for some
time but never got the opportunity. Apparently it took a public mailing
list.
> The idea being that a user could see on the change page (or have a
> dialog pop up if its too expensive to always compute) telling them
> something like:
>
> To submit this change, you need:
>
> Verified +1 from a Verifier. To find a Verifier, click here.
>
> Code Review +2 from a Code Reviewer. To find a Code Reviewer, click here.
>
> DrNo +1 from a DrNoMaster. Required on branch stable. To find a
> DrNoMaster, click here.
Oh, having Gerrit suggest reviewers would be just great.
[...]
--
Magnus B�ck Opinions are my own and do not necessarily
SW Configuration Manager represent the ones of my employer, etc.
Sony Ericsson
Shawn Pearce
> On Thursday, July 01, 2010 at 20:09 CEST,
> Shawn Pearce <s...@google.com> wrote:
>
>> Interesting observation. Currently we don't log who added a
>> ref_right, or when. We should be logging all edits made to the
>> ref_rights table, and as part of that log we should allow you to
>> store an optional short note with the change made. Then you can
>> see what has happened, when, why.
>
> Sounds like a job for Git, doesn't it? I believe Gitosis has a
> configuration git that administrators push changes into and that seems
> like what we need here. It seems to fit very well with the notion of
> moving Gerrit away from the database and into Git.
Yup!
I had at least partially considered that, and it was one of the
reasons I have a side branch that was starting to try and move the
project related metadata into a configuration file stored on a
"hidden" branch inside of each Git repository. Unfortunately it fell
victim to something and got pushed aside. I failed to get back to
that branch. :-|
> Now, this should perhaps not be the default option for admins -- the
> situation for Fredrik and his team is probably fairly unusual. How about
> keeping the existing database table but giving admins an option to make
> the web interface read-only and instead accept configuration file
> changes via Git? This would also open for describing permissions in a
> (more) high-level fashion.
I'd rather just move it all to Git, and have the web interface edit
the Git file for you.
>> As for telling users about why they can't do X, we need to do that
>> better in the UI. I've long had this dream that the access control
>> system should be built on top of a Prolog interpreter rather than
>> being hardcoded in Java, because then we can run the Prolog
>> interpreter to tell us not only "can I do X", but "to do X, what Y
>> must I have"... where Y is the approval bit that is missing, or the
>> group you need to be a member of, or who is a member of the group that
>> has the necessary flag.
>
> Okay, so I wouldn't have suggested Prolog myself but why not?
Well, the Prolog interpreter is just a means to an end. What you
really want here is the inference engine, which can examine the search
space and report to you what paths lead to a particular solution,
especially when there are multiple such paths. Unfortunately, I know
of know such good library in Java. Most that I've seen have optimized
themselves away from a general solution (like an RDF product), are
commercial, or have optimized away the ability to search the full 2^N
space in favor of trying to run in less than exponential time. For
Gerrit, for most users and changes, we can prune that N down enough to
be sufficiently small that a full 2^N search isn't cost prohibitive.
Its still far less time consuming than emailing Fedrik to ask why you
can't do X. :-)
> Apart
> from what Fredrik brought up, one thing that annoys me about the
> current permission configuration is that it's too low-level, concrete,
> and doesn't allow grouping of permissions.
Yes. It also can't be all things to all people, the explosion in
complexity that would result would be horrible. Its hard enough to
understand as it is for users/administrators. But, that said, I don't
believe in limiting power of power users... if you know what you are
doing, the tool should at least let you try and do it. I just want to
try and strike a balance between "its so complex I can't use it" and
"its so simple, we can't express what we want for review workflow".
> For example, our users' default permissions don't include Push Branch +1.
> Everything needs to be reviewed prior to submission. In some cases
> people obtain source code from third parties directly via Git
> or indirectly via git-svn and want to upload those commits to reference
> branches (or upstream branches or whatever you want to call them) that
> are possibly merged to our own branches. This requires the users to have
> Push Branch +2 and Forge Identity +2 for the reference branch so that
> the commits can be pushed as-is. I'd find it so much more elegant if it
> was possible to express this as
>
> group 'acme-import-team' is '3rd-party-importer'
> for ref 'refs/heads/acme/master' in 'platform/vendor/acme'
>
> rather than
>
> acme-team PB +2 refs/heads/acme/master platform/vendor/acme
> acme-team Fg +2 refs/heads/acme/master platform/vendor/acme
>
> (Sorry, don't remember the correct permission group acronyms.)
>
> This would be really convenient if a role suddenly starts to require
> other permissions; you'd just change the role definition and push the
> change to Gerrit, which would convert it to a database representation
> and store it in the same manner as today.
Interesting. Well, if code appears to do this, I can't say I would
object to having the functionality. :-)
Rob Heittman
Most that I've seen have optimized
themselves away from a general solution (like an RDF product), are
commercial, or have optimized away the ability to search the full 2^N
space in favor of trying to run in less than exponential time. For
Gerrit, for most users and changes, we can prune that N down enough to
be sufficiently small that a full 2^N search isn't cost prohibitive.
Its still far less time consuming than emailing Fedrik to ask why you
can't do X. :-)
Shawn Pearce
>> Most that I've seen have optimized
>> themselves away from a general solution (like an RDF product), are
>> commercial, or have optimized away the ability to search the full 2^N
>> space in favor of trying to run in less than exponential time. For
>> Gerrit, for most users and changes, we can prune that N down enough to
>> be sufficiently small that a full 2^N search isn't cost prohibitive.
>> Its still far less time consuming than emailing Fedrik to ask why you
>> can't do X. :-)
>
> This. We had to implement an equivalent advisory function for a customer
> with very complex policy rules. Unfortunately, the lack of a good Java
> library fit for the cause, meant that they ended up with a fairly
> unmaintainable black box that spit out the "allowed because X" or "denied
> because Y" messages. The advisory messages were a big hit with the users,
> the horrid black box of the permission engine is -- as far as I know -- much
> hated by the maintainers to this day.
Exactly. Nobody should hate the code they maintain. In an open
source project its even more important to not hate the code, because
often you have a choice about whether or not you will make changes to
it. With many $DAY_JOBs, your manager can say "do this, or find
another job". With open source, the choice is usually "do this, or go
play with your dog/kids/snowboard". If "this" is horrible, people
choose the latter. :-)
> Now, when you said Prolog before, I facepalmed, since the last time I
> actually wrote any Prolog it was on a bloody Atari. But, now that you
> mention it ... it is going to gnaw at me. Cool idea.
I have had this demo sitting around in a branch on my desktop. It
totally ignores the branch level access we now have (as I wrote it
long before), but its somewhat cool because we can write queries
against it pretty easily. I hate the idea of mixing too many
languages in a single project's source code base (makes contributor
learning curve harder), but... doing this same thing in pure Java is
probably quite a few more lines.
% What do I need to in order to submit the change?
?- submit(p(V,CR)).
P = p(need(verified, 1, 'android-team'), ok(code_review, jbq)) ;
P = p(need(verified, 1, eng), ok(code_review, jbq)) ;
No
% Who is in android-team to get me verified +1?
?- in_group(Who, 'android-team').
Who = bob ;
Who = sally ;
No
--8<--
grant_range(code_review, 'All Users', -1, 1). % all
grant_range(code_review, 'android-admin', -1, 1). % all
grant_range(code_review, 'owners-frameworks-base', -2, 2). % p/f/b
grant_range(verified, 'android-team', -1, 1). % all
grant_range(verified, 'eng', -1, 1).
range_permission(Class, ExpValue, ok(Who)) :-
patch_set_approval(Who, Class, ExpValue),
in_group(Who, Group),
grant_range(Class, Group, Min, Max),
Min @=< ExpValue, ExpValue @=< Max
.
range_permission(Class, ExpValue, ask(Group)) :-
grant_range(Class, Group, Min, Max),
Min @=< ExpValue, ExpValue @=< Max
.
min_max(Class, Min, Max, ok(Class, Who)) :-
\+ range_permission(Class, Min, ok(_)),
range_permission(Class, Max, ok(Who))
.
min_max(Class, Min, Max, reject(Class, Who)) :-
range_permission(Class, Min, ok(Who))
.
min_max(Class, Min, Max, need(Class, Max, Group)) :-
\+ range_permission(Class, Max, ok(_)),
range_permission(Class, Max, ask(Group))
.
min_max(Class, Min, Max, impossible(Class, Max)) :-
\+ range_permission(Class, Max, ask(Group))
.
not_same(ok(A), ok(B)) :- !, A \= B.
not_same(_, _).
verified(Result) :- min_max(verified, -1, 1, Result).
code_review(Result) :- min_max(code_review, -2, 2, Result).
api_council(Result) :- min_max(api_council, -1, 1, Result).
submit(p(V,CR,AC)) :-
modifies_path('api/current.xml'),
!,
verified(V),
code_review(CR),
api_council(AC),
not_same(V, AC).
submit(p(V,CR)) :-
verified(V),
code_review(CR).
% user data
%
in_group(bob, 'android-team').
in_group(sally, 'android-team').
in_group(jbq, 'All Users').
in_group(jbq, 'owners-frameworks-base').
in_group(_, _) :- fail.
% change data
%
modifies_path(_) :- fail.
patch_set_approval(sop, verified, 1).
patch_set_approval(jbq, code_review, 2).
patch_set_approval(_, _, _) :- fail.
cbaldacin
something:
1 - In this first patch we are developing (Rafael and I) we would
rather not going beyond 12th message in this thread since there are
already too many concepts being changed. I believe that from this
patch, we will have more clear scenarios to keep going.
2 - Probably I'm being too simplistic, considering the problem
mentioned by Fredrik (13th message, is there a smarter way to refer a
previous topic?), and apart the prolog idea, it seems to me that it's
just a matter of improve the feedback message whenever the user is
unable to perform something. For example, you should have CodeReview+2
but you have only CodeReview+1. It's not hard to extract such
information from the current database. Still, to improve even more, we
could easily add to the rule the admin user who have created it.
3 - Gerrit does support grouping of permissions. It is supported since
project grouping feature was merged, you can simply set a project as
parent with all desired permissions, then you just set all project you
want to apply the same permissions and set them as children of the
parent.
4 - About prolog, Is the idea to let admin users to create permissions
directly through prolog sentences? Or we would have to translate to a
sentence what the user put in the form?
Carlos
On 1 jul, 19:31, Shawn Pearce <s...@google.com> wrote:
Shawn Pearce
<carloseduar...@sonyericsson.com> wrote:
> well, this discussion evolved a lot, I would like to point out
> something:
>
> 1 - In this first patch we are developing (Rafael and I) we would
> rather not going beyond 12th message in this thread since there are
> already too many concepts being changed. I believe that from this
> patch, we will have more clear scenarios to keep going.
Agreed. Otherwise you may find yourself forever stuck implementing.
I didn't expect you to get much beyond that point anyway right now.
> 2 - Probably I'm being too simplistic, considering the problem
> mentioned by Fredrik (13th message, is there a smarter way to refer a
> previous topic?),
Perhaps hyperlinks to the message in the web archive? I think you can
link to an individual message in a thread.
> and apart the prolog idea, it seems to me that it's
> just a matter of improve the feedback message whenever the user is
> unable to perform something. For example, you should have CodeReview+2
> but you have only CodeReview+1. It's not hard to extract such
> information from the current database. Still, to improve even more, we
> could easily add to the rule the admin user who have created it.
Yes. But there are some more complex cases that the an inference
engine can find for us easily, that are harder to identify without
practically writing the inference engine. For example, I would like
to be able to prove that the current rule set makes it impossible to
submit this change, because nobody has the access required to approve
it. :-)
> 3 - Gerrit does support grouping of permissions. It is supported since
> project grouping feature was merged, you can simply set a project as
> parent with all desired permissions, then you just set all project you
> want to apply the same permissions and set them as children of the
> parent.
Yes, that is true. But its only one inheritance. What if you have
two different types of things you want to inherit? And you now want
to use both of them on this same project? Because we don't support
multiple inheritance you need to duplicate the rules at some point. I
think the grouping idea was to extend it a bit further and essentially
do multiple inheritance, without calling it multiple inheritance.
> 4 - About prolog, Is the idea to let admin users to create permissions
> directly through prolog sentences? Or we would have to translate to a
> sentence what the user put in the form?
Long ago my original idea was to let admin users create some prolog
stanzas themselves, but I think I've come to realize that *most* of
the stanzas they want to write are simple predicate expressions which
we also want to accept as search query strings. So we would probably
translate a string from the admin into a search query predicate tree,
and then translate that into a Prolog expression. Doing a boolean
predicate tree into a Prolog expression tree is pretty simple,
assuming you have operators already defined for different things like
has_label(), in_group(), etc. And my idea for those was to include
them as part of the "Gerrit standard library".
Anyway. Lets only work up through message 12 like you propose and at
least try to get the labeling in place, because that offers a nice
foundation that can be further built on, and solves your original idea
on this thread about just tagging a change with some flag to let you
know its state.
Fredrik Luthander
wrote:
> well, this discussion evolved a lot, I would like to point out
> something:
> 1 - In this first patch we are developing (Rafael and I) we would
> rather not going beyond 12th message in this thread since there are
> already too many concepts being changed. I believe that from this
> patch, we will have more clear scenarios to keep going.
> 2 - Probably I'm being too simplistic, considering the problem
> mentioned by Fredrik (13th message, is there a smarter way to refer a
> previous topic?),
reference. :)
> and apart the prolog idea, it seems to me that it's
> just a matter of improve the feedback message whenever the user is
> unable to perform something. For example, you should have CodeReview+2
> but you have only CodeReview+1. It's not hard to extract such
> information from the current database. Still, to improve even more, we
> could easily add to the rule the admin user who have created it.
for the problem I described. It's like being a developer, I can't ask
you detailed questions about particular code design 6 months after
you've written it, it's too complex and you have forgotten all the
reasons.
So, you need a comment field where we can leave references or other
bits of info as to the reasons of it being there, like the person who
ordered it, or where to turn to get proper access.
> 3 - Gerrit does support grouping of permissions. It is supported since
> project grouping feature was merged, you can simply set a project as
> parent with all desired permissions, then you just set all project you
> want to apply the same permissions and set them as children of the
> parent.
Yes, specific permissions can be inherited from other projects than
All projects.
But Magnus suggested creating if you will "roles" with a certain set
of permissions for common scenarios.
For instance, a regular user would probably be fine with... say, READ
+2 (Upload) and Code review(Db notation 'CRVW')-1..+1.
Magnus point here is to be able to create the user skeleton "regular
user" and give READ+2 and CRVW-1..+1 to this skeleton.
You would then be able to give one group of developers the role/
skeleton of 'user' to project1 for a random ref_pattern.
At the same time you could give a second group of developers the role
of ''user' to project2 for a random ref_pattern.
There are mainly two benefits with this role/skeleton approach;
1. The definition of the user role could mutate over time. Perhaps
users now should have the possibility to give CRVW -2 to a change and
hence be able to block it. To compare two scenarios we then either
have to update all CRVW rules with a -2 for developer/user SGs, but
not for other type of groups that happen to have the same rule in
place. Or, in a second scenario with roles, we could just edit the
role in one place and it would be true for all groups and projects
with users in them.
2. Less rules. Instead of two rules, one READ and one CRVW we would
have one custom rule instead, which in turn would cascade to the two
specific rules. In a scenario with more a complex role, a superuser/
CM, the number of rules drastically becomes fewer, and the risk of
administrative error lessens.
I hope this extended explanation gives you an idea of what Magnus had
in mind. I support his idea fully, it would be great for me and my
teams daily work at $DAY_JOB to have it in place.
BR,
Fredrik
Rafael Rabelo
.
Thanks.
Rafael.
On Jul 2, 3:12 pm, Fredrik Luthander