Dear Gerrit users,
Gerrit let us flag changes with two fields:
- Code Review - Verified
Anyone is allowed to +1/-1 the Code Review field but only a few people are allowed to mark a change as Verified. I have bring back Jenkins in action and it is now running tests for us, whenever a test suite is successful it marks it as Verified.
What that means, is that a change could look fine (Verified + CR) and thus be merged by accident if someone with the correct right click the 'Submit Patch Set 1' button :-D
With bots squatting the Verified field and anyone able to use the Code Review field, Gerrit has an hidden option to add an Approved status. That could be used by the people allowed to merge.
On mediawiki/* we could end up with the following rights:
* Verified : mostly flagged by bots, eventually by human doing manual tests for example when the change has not an automatic test case.
* Code Review : anyone can +1/-1
* Approved : used as a safeguard to prevents people with merge rights to apply a change by mistake.
The feature is used by OpenStack: https://review.openstack.org/
Some examples: https://review.openstack.org/#change,6164 https://review.openstack.org/#change,6196
On Wed, Apr 4, 2012 at 11:55 AM, Antoine Musso hashar+wmf@free.fr wrote:
Dear Gerrit users,
Gerrit let us flag changes with two fields:
- Code Review
- Verified
Anyone is allowed to +1/-1 the Code Review field but only a few people are allowed to mark a change as Verified. I have bring back Jenkins in action and it is now running tests for us, whenever a test suite is successful it marks it as Verified.
What that means, is that a change could look fine (Verified + CR) and thus be merged by accident if someone with the correct right click the 'Submit Patch Set 1' button :-D
No, that's not true. A change requires Code Review +2 before it can be
submitted, not +1. And only trusted reviewers have +2 powers.
The feature is used by OpenStack: https://review.openstack.org/
OpenStack uses this because they have a policy that two core reviewers
must approve (i.e. two +2 reviews are needed) before something can be merged. Because Gerrit allows merges after one +2, they implemented a separate Approved category, and approvals are bureaucratic and based on the presence of two +2s. Or at least that's the impression I got when I talked to the OpenStack people at linux.conf.au .
Overall, I don't think we need this. The only reason I see to implement this is because "+2" is a confusingly named concept, and it would be easier to grok if we just had 3 categories that each just had -1 and +1.
Roan
Le 04/04/12 22:56, Roan Kattouw a écrit : <snip>
What that means, is that a change could look fine (Verified + CR) and thus be merged by accident if someone with the correct right click the 'Submit Patch Set 1' button :-D
No, that's not true. A change requires Code Review +2 before it can be
submitted, not +1. And only trusted reviewers have +2 powers.
So that would prevent me from accidentally submitting some code. :)
OpenStack uses this because they have a policy that two core reviewers
must approve (i.e. two +2 reviews are needed) before something can be merged. Because Gerrit allows merges after one +2, they implemented a separate Approved category, and approvals are bureaucratic and based on the presence of two +2s. Or at least that's the impression I got when I talked to the OpenStack people at linux.conf.au .
Overall, I don't think we need this. The only reason I see to implement this is because "+2" is a confusingly named concept, and it would be easier to grok if we just had 3 categories that each just had -1 and +1.
Indeed, it seems we do not need the Approve right and we can all safely ignore this thread.
Thanks for the explanation!
On 5 April 2012 11:43, Antoine Musso hashar+wmf@free.fr wrote:
Le 04/04/12 22:56, Roan Kattouw a écrit :
<snip> >> What that means, is that a change could look fine (Verified + CR) and >> thus be merged by accident if someone with the correct right click the >> 'Submit Patch Set 1' button :-D >> >> No, that's not true. A change requires Code Review +2 before it can be > submitted, not +1. And only trusted reviewers have +2 powers.
So basically, in Gerrit, (1 + 1 != 2)...?? :-)
--HM
On Thu, Apr 5, 2012 at 4:04 AM, Happy Melon happy.melon.wiki@gmail.comwrote:
So basically, in Gerrit, (1 + 1 != 2)...?? :-)
...and that's exactly why I said that "+2" is a confusingly named concept
in my previous message.
Roan
If you submit a code review for a changeset and not a verification status (only if it is not verified?), it throws you an error:
Application Error Server Error Requires Verified
I suspect it's a bug and we should ignore it but, can we get rid of it? We don't want changesets dummily marked as verified just to please gerrit.
On Thu, Apr 5, 2012 at 2:20 PM, Platonides Platonides@gmail.com wrote:
If you submit a code review for a changeset and not a verification status (only if it is not verified?), it throws you an error:
Application Error Server Error Requires Verified
I suspect it's a bug and we should ignore it but, can we get rid of it? We don't want changesets dummily marked as verified just to please gerrit.
Once we've got jenkins working reliably, I plan to remove the verified permission so only the bots can set it.
-Chad
Hi Chad,
On 2012-04-05, at 2:17 PM, Chad wrote:
Once we've got jenkins working reliably, I plan to remove the verified permission so only the bots can set it.
-Chad
Which languages will Jenkins support?
Best, Diederik
Le 05/04/12 20:20, Diederik van Liere a écrit :
Which languages will Jenkins support?
Jenkins is just a bot, we can make it do whatever we want. The plan is to have a universal linting job able to analyse any language or format in use, be it PHP, Python, JS, CSS ...
https://www.mediawiki.org/wiki/Continuous_integration/Workflow_specification
I am not sure when I am going to work on it, but for sure after we have bring back Testswarm alive.
Thanks, I meant to say what languages initially will be supported :) D On 2012-04-06, at 7:04 AM, Antoine Musso wrote:
Le 05/04/12 20:20, Diederik van Liere a écrit :
Which languages will Jenkins support?
Jenkins is just a bot, we can make it do whatever we want. The plan is to have a universal linting job able to analyse any language or format in use, be it PHP, Python, JS, CSS ...
https://www.mediawiki.org/wiki/Continuous_integration/Workflow_specification
I am not sure when I am going to work on it, but for sure after we have bring back Testswarm alive.
-- Antoine "hashar" Musso
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Le 06/04/12 15:16, Diederik van Liere wrote:
Thanks, I meant to say what languages initially will be supported
Most probably PHP first then Python since you have already provided a linter for it :-D
;)
Le 06/04/12 19:58, Antoine Musso a écrit :
Le 06/04/12 15:16, Diederik van Liere wrote:
Thanks, I meant to say what languages initially will be supported
Most probably PHP first then Python since you have already provided a linter for it :-D
PHP linter is enabled on mediawiki/core.git as of April 8th. It is implemented as a Jenkins job running php -l on any file which have been modified.
Le 07/04/12 09:38, Antoine Musso a écrit :
PHP linter is enabled on mediawiki/core.git as of April 8th. It is implemented as a Jenkins job running php -l on any file which have been modified.
Isn't it also linting merges? It didn't detect the merge today of a broken patchset it had verified before (c4368).
On Thu, Apr 5, 2012 at 11:20 AM, Platonides Platonides@gmail.com wrote:
If you submit a code review for a changeset and not a verification status (only if it is not verified?), it throws you an error:
Application Error Server Error Requires Verified
I suspect it's a bug and we should ignore it but, can we get rid of it? We don't want changesets dummily marked as verified just to please gerrit.
This only happens if you click "Publish and Submit", which means "publish
my comments and attempt to merge the commit". This fails because there is no Verified +1 review, so the commit doesn't satisfy the criteria for merging. If you click "Publish comment" (or whatever the caption of the other publish button is), this doesn't happen.
Roan
On 05/04/12 20:23, Roan Kattouw wrote:
This only happens if you click "Publish and Submit", which means "publish my comments and attempt to merge the commit". This fails because there is no Verified +1 review, so the commit doesn't satisfy the criteria for merging. If you click "Publish comment" (or whatever the caption of the other publish button is), this doesn't happen.
Roan
So in order to comment *and give it a score* you need to choose "Publish comment"? I wouldn't have associated that "submit" as meaning "merge".
On Thu, Apr 5, 2012 at 11:37 AM, Platonides Platonides@gmail.com wrote:
On 05/04/12 20:23, Roan Kattouw wrote:
This only happens if you click "Publish and Submit", which means "publish my comments and attempt to merge the commit". This fails because there is no Verified +1 review, so the commit doesn't satisfy the criteria for merging. If you click "Publish comment" (or whatever the caption of the other publish button is), this doesn't happen.
Roan
So in order to comment *and give it a score* you need to choose "Publish comment"? I wouldn't have associated that "submit" as meaning "merge".
Yes, that's how it works. The Gerrit jargon is totally confusing, I agree.
Roan
wikitech-l@lists.wikimedia.org