There has been some face-to-face discussion about Osmocom's code review process within sysmocom recently. I am posting to this list now with the consent of everyone involved so far, in order to involve the Osmocom community at large in this discussion as well.
There has been a lack of code review from people who don't have "+2" super powers in Gerrit. This applies to anyone among us, independently of any individual's relationship with sysmocom. The bulk of the work involved in reviewing code falls on Harald's shoulders, with Pau and Neels sharing most of the rest between themselves.
At present, while people who add a +1 have their voices heard, their input does not formally affect the decision to merge a change. A change still has to pass by the pre-selected +2 gatekeepers in order to be merged, no matter how many other people have provided input.
The implication for developers without +2 powers is that their time is more effectively spent on advancing their own changes towards a +2 vote, rather than spending time on whatever else is waiting in Gerrit. This may not apply to everyone, of course. But at least for me, this is certainly the case; I have only been reviewing other people's patches when I was explicitly asked to do so.
Myself and several other developers hope that with a change to our review process we can fix this inertia, spread code review across more shoulders and encourage more collaborative code review in Osmocom.
The basic idea is that everyone's input should count for something. If those among us with +1 powers were given a partial say on the fate of every change, our decisions will carry more weight and our influence within the project will slightly increase. We'd also be encouraged to step out of our own corners of expertise every now and then, and look at what other developers are working on. On the flip side, this means we'd carry more responsibility than we do now. We wouldn't always be relying on our +2 gate keepers and would have to apply our own judgement more carefully.
The concrete proposal is to make votes in Gerrit accumulative. Each change would require a total score of +3 to be merged. This score can consist of either a +2 and a +1 vote, or three +1 votes; and no -1 votes. Also, +2 developers would keep their ability to unilaterally block or revert changes under this new model. They'd keep their existing role as arbiters in case of disputes.
Max figured out how Gerrit could be configured for this behaviour. It involves Prolog code, but since we're all quite smart we should be able to figure that out, right? https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html#_e...
We'd be interested to hear what the community thinks about this proposal.
Thanks, Stefan
Hi,
I agree with implementing this kind of change, I really like the idea of getting more people into reviewing and their vote counting so there's no need for somebody with +2 to merge it.
The only drawback I see is the fact that if +3 is needed, then a +2 from some people is now not enough, which can be a pity for patches that can be merged quickly (cosmetic, logging, etc.) or important fixes which require quick merge. Also, for some repos most people don't care bout or don't have much knowledge about it, for instance the case os myself and osmo-gsm-tester, where I usually +2 my patches right away or after leaving them under review for a few hours/days. This would block my development or fix of setup-related issues.
So good for me, but we need to resolve this kind of details in the proposal beforehand.
It involves Prolog code, but since we're all quite smart we should be able to figure that out, right?
From creators of joke "how many developers are required to change a bulb", we now get "How many developers are required to write a prolog program than sums three numbers?"
"How many developers are required to write a prolog program than sums three numbers?"
Answer: none, they are responsible community members.
This works pretty well at subversion.apache.org, where everyone who is accepted as committer (often happens like after the first five patches or so) can directly commit to trunk and wreak havoc if they were so inclined, but since you don't want to stir up the community against you nor want your patches reverted, you adhere to the votes of the others on the ML.
I think just giving +2 permissions to "everyone" is the easiest and socially most beautiful solution: rely on social policy rather than enforcing UI.
That would mean in practice: everyone that is considered a reviewer automatically gets permission to cast +2 votes, and we simply ask everyone to adhere to otiquette ("Osmocom etiquette"), to only give +2 when, say, two others have already cast a +1. If there are any mistakes happening because of that, we can always revert; we can look in gerrit who cast the vote and come back at this person and complain. This also implies that if someone is absolutely sure, she can +2 directly, things get merged and since it is obviously correct, no-one will come back and complain about it. This way would not require an add-up plugin and wouldn't introduce gerrit upgrade problems or prolog error message parsing difficulties or whatnot. Everyone would have the power to fast-track important or trivial patches without bothering anyone else or blocking own work. And everyone will in awe refrain from abusing these powers. If one of the community gurus is convinced, they can still issue immediate +2. If no guru shows up, two or three others can still legitimately reach a +2. Or even, if >=3 reviewers cast a +1, the original poster sees that and decides "that's sufficient" and casts +2 herself to merge.
A minor drawback I see is in the UI: I usually want to quickly see in the overview what patches make sense to take a closer look at now. But even if 10 people cast a +1, the gerrit UI will only show a "+1" in the overview. Also, if one of those is a -1, I think it still shows "+1". So I would welcome listing all the individual votes in the overview list, like "-1,+1x10" -- not sure if that makes sense to implement in a plugin, probably not. Part of this problem goes away when a reviewer casts +2, then the overview will show "+2". But we still lose the "-1" part. A workaround is to always click and open every individual issue... that then shows each single vote of course. I think this minor drawback will still exist if we add up votes: not seeing -1s immediately.
A similar problem I have, independently from above, is that in the overview it is not visible whether my issue had a comment or not. It goes like this: I review a patch, and have a question / remark / tweak. Then I don't really want to vote -1, because that sounds so negative. I don't want to discourage, I rather want to be interested and get a reply. So then I just comment. But on the list of patches, nothing indicates that there is a comment; maybe someone else has voted +2, and the patch gets merged from some other later patch ("Submit including parents") without the comment being seen. Also, if someone cast +2 (or if we add up, when sufficient votes are in), then I still cannot see that there is a -1 and might merge the patch without noticing the question. The only solution now is to click on each individual patch and check one by one whether there are remarks; or to read every single gerrit mail, which I don't do anymore. That's why I prefer that if someone asks even a trivial question on my patch, to please mark -1. Instead, I would love to have some +0 or some '?' indicator in the overview list of patches. One workaround so far was to just issue a +1, which says "yeah, looks ok, but you can't just merge including-all-parents yet, first think about my question on the third patch down". If we add up votes, that would not work anymore, because that +1 already means that I would merge this. Not sure if there is an easy solution.
In summary, I would give all "known users" +2 powers and have an otiquette; Otherwise I'd be ok with trying the add-up plugin.
In all cases I will still have cumbersome issues with the gerrit UI and will have to open each patch individually.
~N
I like what Neels is saying;
* It doesn't look like a technical problem to me, so it probably does not have a technical solution - no change to gerrit required.
I'd put it like this:
1) Everyone, please review more code. (and comment, and +/-1)
2) Code pushers, take a moment to comment your own commits, if it might help other code reviewers.
3) +2er people, take a look at who is doing a lot of code review and consider reaching out to them to invite them to take on +2er responsibility.
k.
Hi Neels,
On Tue, Nov 13, 2018 at 06:16:02PM +0100, Neels Hofmeyr wrote:
I think just giving +2 permissions to "everyone" is the easiest and socially most beautiful solution: rely on social policy rather than enforcing UI.
Makes sense to me. At least for the past and current [size of] community I would consider this a very acceptable/workable solution. Maybe we simply go down that route, rather than setting up more complex technical setups?
The community will have to pay attention that the etiquette is being followed.
A minor drawback I see is in the UI: I usually want to quickly see in the overview what patches make sense to take a closer look at now. But even if 10 people cast a +1, the gerrit UI will only show a "+1" in the overview. Also, if one of those is a -1, I think it still shows "+1". So I would welcome listing all the individual votes in the overview list, like "-1,+1x10" -- not sure if that makes sense to implement in a plugin, probably not. Part of this problem goes away when a reviewer casts +2, then the overview will show "+2". But we still lose the "-1" part. A workaround is to always click and open every individual issue... that then shows each single vote of course. I think this minor drawback will still exist if we add up votes: not seeing -1s immediately.
ACK.
Hi Pau,
On Tue, Nov 13, 2018 at 05:08:49PM +0100, Pau Espin Pedrol wrote:
I agree with implementing this kind of change, I really like the idea of getting more people into reviewing and their vote counting so there's no need for somebody with +2 to merge it.
ACK.
The only drawback I see is the fact that if +3 is needed, then a +2 from some people is now not enough, which can be a pity for patches that can be merged quickly (cosmetic, logging, etc.) or important fixes which require quick merge.
ACK. That's why I think eventually a +3 will be needed. But I'm happy to give the current proposal a (short) try. If we get a backlog of patches at "+2" stage that don't reach "+3" then we must quickly introduce +3.
Also, for some repos most people don't care bout or don't have much knowledge about it, for instance the case os myself and osmo-gsm-tester, where I usually +2 my patches right away or after leaving them under review for a few hours/days. This would block my development or fix of setup-related issues.
Equally ACK here. It's frequent practise in some projects that lack a wider community.
So good for me, but we need to resolve this kind of details in the proposal beforehand.
Indeed, the +3 "privilege" should be configured from day one, with nobody enjoying it until we see a need for it.
Hi.
I really like this idea: merging at +3 seems like a nice balance between not letting code go into master unchecked and having all the reviews by few people only.
If some patch really got to be merged ASAP than I don't think it would be that hard to get extra +1 necessary: just ask at #osmocom :)
I think giving everybody ability to +2 patches and merge them right away defeats the very purpose of having code review system in a first place - we could just merge to master directly in this case.
On Wed, Nov 14, 2018 at 03:04:03PM +0100, Max wrote:
Hi.
I really like this idea: merging at +3 seems like a nice balance between not letting code go into master unchecked and having all the reviews by few people only.
If some patch really got to be merged ASAP than I don't think it would be that hard to get extra +1 necessary: just ask at #osmocom :)
I think giving everybody ability to +2 patches and merge them right away defeats the very purpose of having code review system in a first place - we could just merge to master directly in this case.
I fully agree with Max.
A transition to "everyone has +2" would be a far more drastic change than what I was proposing. I believe this would result in less code review being done than is being done today.
The goal is to get more code review done, not less of it. Requiring +2 voters to also obtain a +1 is part of this idea because it encourages +1 voters to add their votes to changes which already have a +2, whereas today +1 voters tend to ignore changes other than their own.
On Wed, Nov 14, 2018 at 04:16:49PM +0100, Stefan Sperling wrote:
A transition to "everyone has +2" would be a far more drastic change than what I was proposing. I believe this would result in less code review being done than is being done today.
In general, the fact that you can do +2 doesn't mean that you're allowed to do it. The normal case would be: the third reviewer coming along sees that there are already two +1s and then is allowed to vote a +2, by convention / trust.
I would like to adopt the +3 scheme, only I think we don't necessarily need IU enforcement of it by an obscure plugin, because we can agree on it between us humans. Whenever someone disregards "+3", that is a potential offense.
I could trivially enable +2 within minutes right now. No prolog required, only communication. If that doesn't work out, we can still go for the plugin or revert +2 abilities. What do you think?
~N
On Wed, Nov 14, 2018 at 08:44:59PM +0100, Neels Hofmeyr wrote:
On Wed, Nov 14, 2018 at 04:16:49PM +0100, Stefan Sperling wrote:
A transition to "everyone has +2" would be a far more drastic change than what I was proposing. I believe this would result in less code review being done than is being done today.
In general, the fact that you can do +2 doesn't mean that you're allowed to do it. The normal case would be: the third reviewer coming along sees that there are already two +1s and then is allowed to vote a +2, by convention / trust.
I would like to adopt the +3 scheme, only I think we don't necessarily need IU enforcement of it by an obscure plugin, because we can agree on it between us humans. Whenever someone disregards "+3", that is a potential offense.
I could trivially enable +2 within minutes right now. No prolog required, only communication. If that doesn't work out, we can still go for the plugin or revert +2 abilities. What do you think?
~N
I believe that if we're going to use tooling to support a process, then our tooling should be configured to support the process we want. If our tooling is not or cannot be configured as such, we might as well use different tooling.
Social conventions will work but only with continuous effort. Try to think of this as somebody who is looking at Osmocom from the outside without any prior knowledge or involvement, and tries to figure out how we work. The easier it is for that person to figure out answers, the better.
If gerrit does something else than our process says it should, then gerrit makes things harder for new contributors, not easier.
On Thu, Nov 15, 2018 at 01:15:48PM +0100, Stefan Sperling wrote:
I believe that if we're going to use tooling to support a process, then our tooling should be configured to support the process we want.
My point is that this tooling supports our process, no pressing need to enforce it to the last detail.
Social conventions will work but only with continuous effort.
I don't think that will be a problem. There are countless conventions we enforce by social means alone. Coding style, mailing list etiquette... The effort of communicating how we merge patches will be far less frequent than telling people to not send screenshots of text output to the mailing list, it doesn't happen that often that new patch submitters come along.
The easier it is for that person to figure out answers, the better.
Sure, but trying out this scheme without the need to "hack" gerrit is convenient.
If gerrit does something else than our process says it should, then gerrit makes things harder for new contributors, not easier.
If that's really necessary, new people could still get only +1 permissions and everyone that knows what's cooking can get +2? Say, after the fifth patch you get elevated from "known user" to "reviewer" and receive +2 permission.
So, my opinion still is we try it with gerrit as it is:
- vote with +1. - if there are three +1s, the patch owner may apply +2 and merge. - try this, if it doesn't work consider the plugin.
If someone wants to spend effort on installing that plugin, I would be fine with that. I will not be that person because IMHO we don't need it. For me it's quite trivial.
I think my opinion is clear now and I will refrain from rephrasing it again. If you agree please say so, so that I can configure gerrit +2 permissions. If you disagree, as far as I'm concerned someone go on and install that plugin.
Otherwise we have stalemate and will keep the current scheme.
~N
Hi,
while I'd like it to be the opposite, i tend to think you cannot trust human being until they proved you can do so -> I'm against giving +2 to everybody by default.
I agree with Stefan and I wouldn't go for a too drastic change, let's first move to something like the approach described initially: 3*+1 or 1*+2can merge a patch. If it makes it easier to deploy it, then also make it 3 points however combined can merge a patch (meaning nobody can merge on its own, despite I think this may affect productivity having some patches merged).
Regards, Pau
On Thu, Nov 15, 2018 at 03:39:36PM +0100, Pau Espin Pedrol wrote:
Hi,
while I'd like it to be the opposite, i tend to think you cannot trust human being until they proved you can do so -> I'm against giving +2 to everybody by default.
I don't see any cause for concern there. We have version control.
Nobody can undo the main line of history. Mistakes will always be made, and can always be undone.
I agree with Stefan and I wouldn't go for a too drastic change,
Indeed, I designed my proposal with minimal changes to the existing process in mind. But it seems like Neels and my own proposal aren't far apart, actually. The only difference is whether our self-enforced boundaries are enforced by technical or social means, really.
On Thu, Nov 15, 2018 at 07:07:34PM +0100, Stefan Sperling wrote:
Nobody can undo the main line of history.
Well, some of us can, since we have direct write permission on the git repositories in gerrit.
(And my point is that we don't use that capability due to social convention.)
But it seems like Neels and my own proposal aren't far apart, actually. The only difference is whether our self-enforced boundaries are enforced by technical or social means, really.
Exactly!
~N
On Thu, Nov 15, 2018 at 03:31:11PM +0100, Neels Hofmeyr wrote:
So, my opinion still is we try it with gerrit as it is:
- vote with +1.
- if there are three +1s, the patch owner may apply +2 and merge.
- try this, if it doesn't work consider the plugin.
OK, I see. When phrased like this, your suggestion doesn't sound difficult to realize. So I would be happy to try this. After all, the end result is the same as with my proposal, modulo concerns about giving everyone the power to make a 'Submit' button appear by default. And I don't have any concerns about that.
Hi.
I second this: rules have to be enforced by technical means whenever possible. Social conventions simply don't scale: if we let people just merge it arbitrary than there'll be never-ending stream of "I know I shouldn't have but..." with all sorts of "buts" (I was in a hurry, I was about to leave, I needed for X, etc).
It might work fine in a close-knit group but I think the goal is to grow the number of contributions by making it as effortlessly as possible. It's always better than newcomers simply can't do the wrong thing rather than constantly reminding them what is right thing, on which wiki (which inevitably will get outdated) they can read about it etc.
Also, I disagree that setting up gerrit this way is a "hack": making votes additive is described in official documentation for gerrit. It's supported by upstream - there's nothing hackish about it.
On 15/11/2018 15:41, Max wrote:
Hi.
I second this: rules have to be enforced by technical means whenever possible.
wow.
It's interesting this should come up right now - I've recently been spending a lot of time reading about, watching documentaries, (last night it was the Bolsheviks!!) and generally studying the whole concept of this machine controlled society we (tech people) are blindly making for ourselves. And it's certainly one in which do not want to live, nor do I want anybody else I care about to live in.
As said Neels, I'll refrain from expressing my opinion again after this, so just once more; A "community" that can operate without the technical enforcement of rules will be a more healthy community.
The other way will come back and bite you, eventually.
"I know I shouldn't have but..."
A little more on topic:
I count >30 known users on gerrit. Do we all have +1 ability? Then it wouldn't be hard to see this technical solution just create division, or at the very least consume time of those who have to be vigilantes and -1 some things in time.
I think there might be some misunderstanding worth clarifying in here.
15.11.18 18:43, Keith пишет:
...and generally studying the whole concept of this machine controlled society we (tech people) are blindly making for ourselves. And it's certainly one in which do not want to live, nor do I want anybody else I care about to live in.
Right now the rules for merging patches are enforced by Gerrit.
What Neels proposes is a model where people manually abide by those rules.
I think this change is too drastic and that we should keep existing model, only changing the rules (see $subj) slightly.
On 15/11/2018 19:07, Max wrote:
I think there might be some misunderstanding worth clarifying in here.
:) Don't take the fact I happened to reply to you to mean anything.. although, well it's some powerful language, that struck me.. today..
Stefan puts it well though, although I'm thinking the little things we do that are "just" our model for X in tech seem to be bleeding over too much into other areas of life.
politics and code.. can't have one without the other.. and I am actually also subscribed to more suitable mailing lists for this kind of stuff..
Meanwhile, I'll endeavor to do more code review. :))
On Thu, Nov 15, 2018 at 06:43:20PM +0100, Keith wrote:
On 15/11/2018 15:41, Max wrote:
Hi.
I second this: rules have to be enforced by technical means whenever possible.
wow.
It's interesting this should come up right now - I've recently been spending a lot of time reading about, watching documentaries, (last night it was the Bolsheviks!!) and generally studying the whole concept of this machine controlled society we (tech people) are blindly making for ourselves. And it's certainly one in which do not want to live, nor do I want anybody else I care about to live in.
But in this case, we make up the rules ourselves. We voluntarily impose mandatory code review upon ourselves because we believe this helps us ship better software at the end of the day. Because we all make mistakes.
Contrary to problems with tech in society in general, we don't impose our code review process onto people who aren't directly involved. And it's entirely up to us how far we go with enforcing our self-made rules, by technical means or otherwise.
So I don't think this analogy makes sense in this particular case.
As said Neels, I'll refrain from expressing my opinion again after this, so just once more; A "community" that can operate without the technical enforcement of rules will be a more healthy community.
The community is healthy if this discussion ends up with consensus for some particular code review process we decide to try next. We'll only get there if people keep expressing opinions, listen to each other, and compromise willingly. So please don't stop voicing your opinions.
On 11/15/18 7:19 PM, Stefan Sperling wrote:
So please don't stop voicing your opinions.
For what it's worth, I would prefer to enforce the rule as well. Even if it does not seem like much effort to explain this workflow to new users, and to ask people to think about the +3 before merging: these things add up, leave room for mistakes and scale badly (as already pointed out in this thread).
It's the same with CI that builds code and runs test cases - of course we could trust everybody to do this on their own before publishing patches, but that is not as good as checking it automatically and having proof that the tests really ran through before merging.
Best regards, Oliver
Hi Stefan,
On Tue, Nov 13, 2018 at 04:43:50PM +0100, Stefan Sperling wrote:
There has been some face-to-face discussion about Osmocom's code review process within sysmocom recently. I am posting to this list now with the consent of everyone involved so far, in order to involve the Osmocom community at large in this discussion as well.
Thanks for getting the discussion going here.
There has been a lack of code review from people who don't have "+2" super powers in Gerrit.
Actually, to be more precise there also has been alack of code review from many people who already had +2 power. I understand your line of thought and your arguments, but let's try to state all the facts :)
I'm losing track of opinions, so I made this poll: https://dudle.inf.tu-dresden.de/voting_on_gerritosmocomorg/
It is easier to see how many would accept what to come to terms with what we want. (If the wording is confusing we can edit the poll.)
Thanks! ~N
On Mon, Nov 19, 2018 at 03:04:23PM +0100, Neels Hofmeyr wrote:
Intermediate summary: it's a very close run between "social convention only" (3 acks) and "UI enforced +3, no-one has +3 rights" (3 acks plus one '?', say 3.5). Er, should I subtract the red 'x' votes? In that case "UI enforced, no-one has +3" would win 2.5 against 1. Let's wait another few days, maybe more wish to submit their vote.
About "no-one has +3", I will dislike not being able to fast-track the odd patch here and there, and will probably use my direct write-to-git permission, instead of waiting and wasting other people's time on, say, whitespace changes.
On the poll page, a question was asked by "Anonymous", am hereby taking the discussion back here:
I don't understand why you insist on calling configuration a plugin. There's no need to install anything which isn't already there. We have to change configuration according to the documentation. What "plugin" or "installation" are you referring to?
IIUC introducing adding up of +1 votes requires some prolog script to be added/installed/enabled in gerrit. Last time I asked whether anyone had figured out how to do it the answer was a resounding "?".
To say it yet over again: whatever it takes, If someone is willing to make gerrit behave that way, fine with me. But I will not be that person.
~N
Looking at the dudle, I now see that clearly this is your favorite:
"merge at three votes, by enforcing from gerrit: we install a plugin to add up voting, +3 allows to merge, at first no-one has +3 permissions"
So, we need a volunteer to try out that prolog snippet Max pointed out: https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html#_e...
I'd suggest first in a local sandbox gerrit. We might even have an ansible to setup our gerrit which could be useful to create a testing instance, not sure. If we do, someone please point to it here.
If you have it figured out and tested, get access and install it on our production gerrit. Ideal would be a scripted/ansible installation. Also ideal would be to make sure a backup has run recently before you start.
The strongest proponents seem to be Max, pau, osmith, daniel. Thanks!
Once this is in place, we will probably have a discussion about whether some people get direct +3 permissions (matching the request for another option "some people have +3 permissions" seen in the dudle comments).
I'd prefer if this would happen sooner rather than later. Still this week maybe?
~N
On 11/28/18 12:04 AM, Neels Hofmeyr wrote:
Looking at the dudle, I now see that clearly this is your favorite:
"merge at three votes, by enforcing from gerrit: we install a plugin to add up voting, +3 allows to merge, at first no-one has +3 permissions"
So, we need a volunteer to try out that prolog snippet Max pointed out: https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html#_e...
I took a shot at this. Reading up on the docs, I was surprised that the prolog code does not go into the gerrit installation. But instead into the git repository along with the code. From the top of that docs site:
$ git fetch origin refs/meta/config:config $ git checkout config ... edit or create the rules.pl file $ git add rules.pl $ git commit -m "My submit rules" $ git push origin HEAD:refs/meta/config
Obviously not everybody is allowed to do this. I get (after making sure that the remote url points to gerrit, not git.osmocom.org):
fatal: Couldn't find remote ref refs/meta/config
So my suggestion is, that somebody who has the permissions, tries to push the rules.pl file to one not-so-important Osomocom repository for test purposes. And then we simply try it out.
Regarding the contents of the rules.pl file, it seems we only need to change "2" to "3" in the following line of the given example. Counting to three is supposed to be easy after all.
add_category_min_score(NoCR,'Code-Review', 2, Labels),
The docs also say (on top) that these rules may be disabled with "rules.enable=false" in the gerrit config. It does not sound like that is the default, but in case it is, we would need to provide a '$site_path'/etc/gerrit.config file that overrides this.
My understanding is, that gerrit.osmocom.org runs the docker container from docker-playground.git/gerrit. I've patched it to build again, and I can add the config file there if necessary.
https://gerrit.osmocom.org/#/q/topic:docker-gerrit-fixes
Regards, Oliver
On Fri, Dec 07, 2018 at 04:36:27PM +0100, Oliver Smith wrote:
So my suggestion is, that somebody who has the permissions, tries to push the rules.pl file to one not-so-important Osomocom repository for test purposes. And then we simply try it out.
the sandbox repos on gerrit ssh://you@gerrit.osmocom.org:29418/sandbox
I gave you all sorts of permissions on the sandbox repos, but then figured, if I forgot something, I might as well give you admin access on the gerrit ui as well.
My understanding is, that gerrit.osmocom.org runs the docker container from docker-playground.git/gerrit. I've patched it to build again, and I can add the config file there if necessary.
hmm, no idea.
~N
On 12/8/18 3:31 PM, Neels Hofmeyr wrote:
On Fri, Dec 07, 2018 at 04:36:27PM +0100, Oliver Smith wrote:
So my suggestion is, that somebody who has the permissions, tries to push the rules.pl file to one not-so-important Osomocom repository for test purposes. And then we simply try it out.
the sandbox repos on gerrit ssh://you@gerrit.osmocom.org:29418/sandbox
I gave you all sorts of permissions on the sandbox repos, but then figured, if I forgot something, I might as well give you admin access on the gerrit ui as well.
Thank you! It worked as expected. Here's a patch, that has been merged with 3x +1 votes. It would also have been possible to submit with 1x +1 and 1x +2 (as tnt clicked first), but not without reaching 3 in total.
https://gerrit.osmocom.org/#/c/sandbox/+/12216/
For the record, here are the steps again (now needs to be applied to every Osmocom project repo):
$ git fetch origin refs/meta/config:config $ git checkout config ... add rules.pl from the example, replace 2 with 3 $ git add rules.pl $ git commit -m "My submit rules" $ git push origin HEAD:refs/meta/config
It seems that I have permissions now to do this to all projects. Should I go ahead?
Regards, Oliver
Hi.
I think there's one more thing we should test first - see below.
10.12.18 13:30, Oliver Smith пишет:
It seems that I have permissions now to do this to all projects. Should I go ahead?
What about -1 vote counting?
Say User1 put -1 on the patch, User2 put +1 and User3 put +2. Can the patch be merged in this case?
Essentially, does substraction works as well as deletion?
Hi Max and ML,
On 12/10/18 1:39 PM, Max wrote:
What about -1 vote counting?
Say User1 put -1 on the patch, User2 put +1 and User3 put +2. Can the patch be merged in this case?
We played with this idea here: https://gerrit.osmocom.org/#/c/sandbox/+/12218/
Results: * -1, +1, +2 -> can not be merged * -1, +1, +2, +1 -> can be merged
So yes, negative votes do get substracted from the total of 3 that is required for a merge.
However, Neels discovered something else we should think about collectively: the overview in the UI is confusing with the new +3 configuration applied (which we currently have in the sandbox repo only).
https://gerrit.osmocom.org/#/q/%22What+about+-1+vote+counting%22
As time of writing, this shows only the patch mention above. It can *not* be merged, as it has -1+1+2=2. However, the search results show a green checkmark in the code review (CR) column. Probably, because there is a +2 in the reviews. So this is misleading, as one might think that the patch is ready, although it isn't.
The "New UI" is only slightly better. It is also showing the checkmark, but it is colored red instead of green.
So... is this what we want, or should we rather give everyone +2 and set up a social convention (see earlier discussions in this thread)?
Regards, Oliver
Thanks for taking care of this.
10.12.18 14:25, Oliver Smith пишет:
The "New UI" is only slightly better. It is also showing the checkmark, but it is colored red instead of green.
What is "New UI"?
So... is this what we want, or should we rather give everyone +2 and set up a social convention (see earlier discussions in this thread)?
I'm pretty sure that it's possible to fix the display to match the expectations, however it's only cosmetic issue (as long as "submit" button does not appear) so I don't think it should be a showstopper.
On 12/10/18 2:41 PM, Max wrote:
What is "New UI"?
There's a link at the bottom of the site, "Switch to New UI": https://gerrit.osmocom.org/?polygerrit=1
Looks neat, thanks.
I've also noticed that we're not using the latest gerrit release where this UI is default. Could it be that displaying checkbox symbol is fixed in there?
10.12.18 14:51, Oliver Smith пишет:
On 12/10/18 2:41 PM, Max wrote:
What is "New UI"?
There's a link at the bottom of the site, "Switch to New UI": https://gerrit.osmocom.org/?polygerrit=1
On 12/10/18 3:11 PM, Max wrote:
I've also noticed that we're not using the latest gerrit release where this UI is default.
Good catch, it seems they are planning to deprecate the old UI soon.
Could it be that displaying checkbox symbol is fixed in there?
Maybe, but I don't spot anything related in their release notes:
https://www.gerritcodereview.com/2.16.html
I am not even sure if they would consider it a bug. The color changes between green and red in the new UI after all, seems like it is intentional.
Personally, I would like to have the +3 thing implemented in every repository, like it is done now in the sandbox repo, even if the old UI displays it a bit weird.
Any more opinions on this?
Regards, Oliver
Let's move forward, +1 (we have +3 already, can we merge? :-P)
On Mon, Dec 10, 2018 at 02:51:08PM +0100, Oliver Smith wrote:
On 12/10/18 2:41 PM, Max wrote:
What is "New UI"?
There's a link at the bottom of the site, "Switch to New UI": https://gerrit.osmocom.org/?polygerrit=1
The single worst issue in the "new UI" for me is that when a patch is in "WIP" mode, it is impossible "Reply" and send the draft comments. For that I need to go back to the old UI.
I also think some design choices are too thin/flimsy and "hard to see", the old ui has more contrast and substance.
I tried to send feedback on it, but doing so requires a google login or something equally useless, and they don't just provide an email address. So there I wasn't willing to drill further...
Otherwise the new UI is a huge improvement in that each patch shows the related patches *in the right sequence*, and on the summary page there is a row highlight on mouse hover, so it's 1000x easier to click the right row. Also the new UI remembers the patch reply text even if you click on an in-file draft comment and come back later, where the old UI obliterates your text.
Remembering the UI choice seems to be a cookie, so if you remove cookies on browser closing, you will always be thrown back to the old ui.
...so much for that :)
~N
I think we can give it a try and see whether the annoyance of the below is annoying enough to bother:
On Mon, Dec 10, 2018 at 02:25:32PM +0100, Oliver Smith wrote:
However, Neels discovered something else we should think about
in summary, even with the +3 voting config applied:
A patch with CR votes | shows on the patch summary page as | old UI | new UI ------------------------------------------------------------------ +1 | +1 | +1 +1 +1 | +1 | +1 +1 +1 +1 | +1 | +1 (!) +2 | green tick | green tick (!) +2 +1 | green tick | green tick +2 -1 | green tick | red tick
(I hope that's accurate)
Also I found that the "label status" shown in the new UI when viewing the patch isn't accurate, only the presence of the "Submit" button is.
I do think it sucks that it doesn't show the status properly; you always need to click around and manually sum up CR votes to see where you're at. It's quite a half-arsed implementation, little more than a quick example. Maybe that prolog code can be fixed to reflect a more accurate or custom label status? (Or a social convention would solve that in a breeze.)
Also I expect patches to sit around a lot longer, because getting three code reviews so far was quite rare. And I guess it's not ok to add an own +1 vote? :) Maybe we'll need to reduce +3 to +2. Well, let's see.
But ok to give it a try as-is.
Oh, BTW, do some people still get the right to +2, so that actually only two reviews are needed if one of them voted +2? Or does everyone only +1 now? (easy to configure and enforce)
~N
On Tue, Dec 11, 2018 at 12:44:34AM +0100, Neels Hofmeyr wrote:
I do think it sucks that it doesn't show the status properly; you always need to click around and manually sum up CR votes to see where you're at. It's quite a half-arsed implementation, little more than a quick example.
Well... We are doing code review via a *web interface* So what else would you expect?
Oh, BTW, do some people still get the right to +2, so that actually only two reviews are needed if one of them voted +2? Or does everyone only +1 now? (easy to configure and enforce)
Current +2 / +1 vote token assignments shouldn't change. This was never explicitly discussed, but it was implied.
On Tue, Dec 11, 2018 at 08:58:53AM +0100, Stefan Sperling wrote:
So what else would you expect?
I expect this huge stack of server and client both running on sheer godlike products of science and maths to be able to count to three! :) All this logic is expertly stacked and designed to produce ... garble.
But right, that's pretty much the definition of web apps.
/me reminisces the days of <table> websites and when simple weblinks and scroll bars worked without javacript ... back when gmx.net was cool :)
Oh, BTW, do some people still get the right to +2, so that actually only two reviews are needed if one of them voted +2? Or does everyone only +1 now? (easy to configure and enforce)
Current +2 / +1 vote token assignments shouldn't change. This was never explicitly discussed, but it was implied.
Ok, then it might turn out to be not such dramatic delay in patches...
~N
If you dislike web interfaces so much than why do you keep using it instead of something like https://github.com/openstack/gertty ?
11.12.18 12:10, Neels Hofmeyr пишет:
But right, that's pretty much the definition of web apps.
On Tue, Dec 11, 2018 at 12:43:31PM +0100, Max wrote:
If you dislike web interfaces so much than why do you keep using it instead of something like https://github.com/openstack/gertty ?
interesting, I think I knew this once but forgot about it.
I wonder, how does gertty interact with the +3 scheme...
~N
On Tue, Dec 11, 2018 at 12:43:31PM +0100, Max wrote:
of something like https://github.com/openstack/gertty ?
All that gertty is giving me is "Error" in the top right corner without the slightest hint at what error it might be. Ah, there's a ~/.gertty.log.
Hm, just python tracebacks: AttributeError: 'NoneType' object has no attribute 'keys'
Well, not willing to fix their code as well, web interface it is then.
~N
So, everbody who replied lately was in favor of rolling out this rule, as we had tested it in the sandbox. I'm rolling it out now.
On 12/12/18 12:19 PM, Oliver Smith wrote:> So, everbody who replied lately was in favor of rolling out this rule,
as we had tested it in the sandbox. I'm rolling it out now.
Done. I've updated the "Adding a new repository" section in the Gerrit wiki page with instructions on adding the rules.pl file:
https://osmocom.org/projects/cellular-infrastructure/wiki/Gerrit
-
Harald Welte -
Keith -
Max -
Neels Hofmeyr -
Oliver Smith -
Pau Espin Pedrol -
Stefan Sperling