13 Nov
2018
13 Nov
'18
4:43 p.m.
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#_…
We'd be interested to hear what the community thinks about this proposal.
Thanks,
Stefan
13 Nov
13 Nov
5:08 p.m.
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?"
--
- Pau Espin Pedrol <pespin(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
6:16 p.m.
"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
14 Nov
14 Nov
4:08 p.m.
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.
19 Nov
19 Nov
7:36 a.m.
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.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
7:33 a.m.
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.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
14 Nov
14 Nov
3:04 p.m.
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.
--
- Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Harald Welte
4:16 p.m.
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.
8:44 p.m.
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
15 Nov
15 Nov
1:15 p.m.
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:
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.
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
3:31 p.m.
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
3:39 p.m.
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
--
- Pau Espin Pedrol <pespin(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
7:07 p.m.
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.
19 Nov
19 Nov
3 p.m.
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
15 Nov
15 Nov
7:03 p.m.
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.
3:41 p.m.
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.
--
- Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Harald Welte
6:43 p.m.
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.
7:07 p.m.
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.
--
- Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Harald Welte
8:08 p.m.
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. :))
7:19 p.m.
On Thu, Nov 15, 2018 at 06:43:20PM +0100, Keith wrote:
On 15/11/2018 15:41, Max wrote:
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.
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 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.
16 Nov
16 Nov
10:47 a.m.
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
--
- Oliver Smith <osmith(a)sysmocom.de> https://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
19 Nov
19 Nov
7:30 a.m.
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 :)
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
3:04 p.m.
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
20 Nov
20 Nov
1 a.m.
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
28 Nov
28 Nov
12:04 a.m.
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#_…
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
7 Dec
7 Dec
4:36 p.m.
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#_…
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
--
- Oliver Smith <osmith(a)sysmocom.de> https://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
8 Dec
8 Dec
3:31 p.m.
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
10 Dec
10 Dec
1:30 p.m.
On 12/8/18 3:31 PM, Neels Hofmeyr wrote:
On Fri, Dec 07, 2018 at 04:36:27PM +0100, Oliver Smith
wrote:
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
--
- Oliver Smith <osmith(a)sysmocom.de> https://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
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.
1:39 p.m.
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?
--
- Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Harald Welte
2:25 p.m.
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
--
- Oliver Smith <osmith(a)sysmocom.de> https://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
2:41 p.m.
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.
--
- Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Harald Welte
2:51 p.m.
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
--
- Oliver Smith <osmith(a)sysmocom.de> https://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
3:11 p.m.
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:
--
- Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Harald Welte
What is "New UI"?
There's a
link at the bottom of the site, "Switch to New UI":
https://gerrit.osmocom.org/?polygerrit=1
3:48 p.m.
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
--
- Oliver Smith <osmith(a)sysmocom.de> https://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
3:58 p.m.
Let's move forward, +1 (we have +3 already, can we merge? :-P)
--
- Pau Espin Pedrol <pespin(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
11 Dec
11 Dec
12:13 a.m.
On Mon, Dec 10, 2018 at 02:51:08PM +0100, Oliver Smith wrote:
On 12/10/18 2:41 PM, Max wrote:
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
What is "New UI"?
There's a link at the bottom of the site, "Switch to New UI":
https://gerrit.osmocom.org/?polygerrit=1
12:44 a.m.
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
8:58 a.m.
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.
12:10 p.m.
On Tue, Dec 11, 2018 at 08:58:53AM +0100, Stefan Sperling wrote:
Ok, then it might turn out to be not such dramatic delay in patches...
~N
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.
12:43 p.m.
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.
--
- Max Suraev <msuraev(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Harald Welte
12 Dec
12 Dec
12:11 p.m.
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
12:21 p.m.
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
12:19 p.m.
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.
--
- Oliver Smith <osmith(a)sysmocom.de> https://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
12:41 p.m.
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
--
- Oliver Smith <osmith(a)sysmocom.de> https://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
2423
days inactive
2452
days old
43 comments
7 participants
participants (7)
-
Harald Welte -
Keith -
Max -
Neels Hofmeyr -
Oliver Smith -
Pau Espin Pedrol -
Stefan Sperling