[prev in list] [next in list] [prev in thread] [next in thread]
List: vdsm-devel
Subject: =?utf-8?q?=5Bovirt-devel=5D?= Re: Merge rights changes in the oVirt Engine project
From: Michal Skrivanek <michal.skrivanek () redhat ! com>
Date: 2021-04-09 13:25:14
Message-ID: B07AE56E-AA74-4291-BF8F-EA18B4B9EEAD () redhat ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On 8. 4. 2021, at 7:18, Yedidyah Bar David <didi@redhat.com> wrote:
>
> On Wed, Apr 7, 2021 at 6:45 PM Michal Skrivanek
> <michal.skrivanek@redhat.com <mailto:michal.skrivanek@redhat.com>> wrote:
> >
> >
> >
> > > On 7. 4. 2021, at 12:04, Yedidyah Bar David <didi@redhat.com> wrote:
> > >
> > > On Tue, Aug 11, 2020 at 11:05 AM Tal Nisan <tnisan@redhat.com> wrote:
> > > >
> > > > Hi everyone,
> > > > As you probably know we are now in a mode in which we develop our next \
> > > > zstream version on the master branch as opposed to how we worked before where \
> > > > the master version was dedicated for the next major version. This makes the \
> > > > rapid changes in master to be delivered to customers in a much higher cadence \
> > > > thus affecting stability. Due to that we think it's best that from now on \
> > > > merges in the master branch will be done only by stable branch maintainers \
> > > > after inspecting those closely.
> > > > What you need to do in order to get your patch merged:
> > > > - Have it pass Jenkins
> > > > - Have it get code review +2
> > > > - Have it mark verified +1
> > > > - It's always encourage to have it tested by OST, for bigger changes it's a \
> > > > must
> > > > Once you have all those covered, please add me as a reviewer and I'll examine \
> > > > it and merge if everything seems right, if I haven't done it in a timely \
> > > > manner feel free to ping me.
> > >
> > > Is the above still the current policy?
> >
> > Hi Didi,
> > well, yes it is. what's the concern?
>
> No "concern", other than I noticed a few times that people other than
> Tal merged patches, and yesterday I did the same [1] after getting +1
that's master branch, not stable branch.
> from Sandro and consulting him, seeing that I have permissions. IIRC I
> didn't have permissions until recently, so I wondered if anything
> changed.
>
> If the re-granting of permissions was a mistake, let's revert. If it's
> on purpose, perhaps clarify the situation.
we did a major cleanup of stale people and the convoluted system of groups that \
accumulated in the past we now just use a simple "regular" and stable maintainers \
list. +2 are for respective areas, we do not have a per-area granularity in \
ovirt-engine project, it's really just an agreement that you shouldn't merge patches \
in areas that are not yours.
oh, no w I get it, you really talk about submitting, not +2:) ok, yes, so that has \
changed. we've concluded that we can go back to previous mode of not distinguishing \
between +2 and merge rights. everything else above applies about the patch quality \
criteria, it's just that indeed anyone in this list[1] can click the submit button.
>
> [1] https://gerrit.ovirt.org/c/ovirt-engine/+/114130 \
> <https://gerrit.ovirt.org/c/ovirt-engine/+/114130>
> > I'd love to get to a point when we can automatically gate patches based on OST, \
> > but it's going slow…so for now it's still manual.
>
> Not sure that's enough, but it would be a step in the right direction.
> Sometimes patches won't break OST but are still harmful.
sure, it's never 100%, still better than today, especially since we started to pay \
more attention to OST results it's not only that it brings in a regression, it's also \
all that wasted time by people trying to fix OST after such patch.
>
> Did we ever consider going fully to Test-Driven-Development? Not
> certain if there are studies/methods to calculate/approximate the
> expected extra work this will require. Assuming you want eventually
> all developers to also know well-enough OST (in addition to their
> specific expertise) and be comfortable patching it, it might make
> sense.
I think anything more involved would be complicated by the slowness. It's better than \
before, but still ~35 minutes at best for basic suite. And it's fragile so not really \
that simple to add a test without either breaking it or making it too isolated - too \
slow for practical use. There are always unit tests of course, that's a separate \
matter...
Thanks,
michal
>
> Best regards,
> —
> Didi
[1] https://gerrit.ovirt.org/#/admin/groups/63,members
[Attachment #5 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html; \
charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; \
line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote \
type="cite" class=""><div class="">On 8. 4. 2021, at 7:18, Yedidyah Bar David <<a \
href="mailto:didi@redhat.com" class="">didi@redhat.com</a>> wrote:</div><br \
class="Apple-interchange-newline"><div class=""><meta charset="UTF-8" class=""><span \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; \
float: none; display: inline !important;" class="">On Wed, Apr 7, 2021 at 6:45 PM \
Michal Skrivanek</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; \
font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: \
Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; \
font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; \
text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline \
!important;" class=""><</span><a href="mailto:michal.skrivanek@redhat.com" \
style="font-family: Helvetica; font-size: 12px; font-style: normal; \
font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: \
auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; \
widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; \
-webkit-text-stroke-width: 0px;" class="">michal.skrivanek@redhat.com</a><span \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; \
float: none; display: inline !important;" class="">> wrote:</span><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; \
-webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""><br \
class=""><br class=""><blockquote type="cite" class="">On 7. 4. 2021, at 12:04, \
Yedidyah Bar David <<a href="mailto:didi@redhat.com" \
class="">didi@redhat.com</a>> wrote:<br class=""><br class="">On Tue, Aug 11, 2020 \
at 11:05 AM Tal Nisan <<a href="mailto:tnisan@redhat.com" \
class="">tnisan@redhat.com</a>> wrote:<br class=""><blockquote type="cite" \
class=""><br class="">Hi everyone,<br class="">As you probably know we are now in a \
mode in which we develop our next zstream version on the master branch as opposed to \
how we worked before where the master version was dedicated for the next major \
version. This makes the rapid changes in master to be delivered to customers in a \
much higher cadence thus affecting stability.<br class="">Due to that we think it's \
best that from now on merges in the master branch will be done only by stable branch \
maintainers after inspecting those closely.<br class=""><br class="">What you need to \
do in order to get your patch merged:<br class="">- Have it pass Jenkins<br \
class="">- Have it get code review +2<br class="">- Have it mark verified +1<br \
class="">- It's always encourage to have it tested by OST, for bigger changes it's a \
must<br class=""><br class="">Once you have all those covered, please add me as a \
reviewer and I'll examine it and merge if everything seems right, if I haven't done \
it in a timely manner feel free to ping me.<br class=""></blockquote><br class="">Is \
the above still the current policy?<br class=""></blockquote><br class="">Hi Didi,<br \
class="">well, yes it is. what's the concern?<br class=""></blockquote><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: \
12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none; float: none; display: inline !important;" class="">No \
"concern", other than I noticed a few times that people other than</span><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: \
12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none; float: none; display: inline !important;" class="">Tal merged \
patches, and yesterday I did the same [1] after getting +1</span><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
class=""></div></blockquote><div><br class=""></div>that's master branch, not stable \
branch. </div><div><br class=""><blockquote type="cite" class=""><div \
class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: \
12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none; float: none; display: inline !important;" class="">from Sandro \
and consulting him, seeing that I have permissions. IIRC I</span><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: \
12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none; float: none; display: inline !important;" class="">didn't have \
permissions until recently, so I wondered if anything</span><br style="caret-color: \
rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; \
font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: \
start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: \
0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; \
float: none; display: inline !important;" class="">changed.</span><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: \
12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: \
Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; \
font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; \
text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline \
!important;" class="">If the re-granting of permissions was a mistake, let's revert. \
If it's</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; \
font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: \
Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; \
font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; \
text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline \
!important;" class="">on purpose, perhaps clarify the situation.</span><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
class=""></div></blockquote><div><br class=""></div>we did a major cleanup of stale \
people and the convoluted system of groups that accumulated in the past</div><div>we \
now just use a simple "regular" and stable maintainers list. +2 are for respective \
areas, we do not have a per-area granularity in ovirt-engine project, it's really \
just an agreement that you shouldn't merge patches in areas that are not \
yours.</div><div><br class=""></div><div>oh, no w I get it, you really talk about \
submitting, not +2:) ok, yes, so that has changed. we've concluded that we can go \
back to previous mode of not distinguishing between +2 and merge \
rights.</div><div>everything else above applies about the patch quality criteria, \
it's just that indeed anyone in this list[1] can click the submit \
button.</div><div><br class=""></div><div><br class=""></div><div><blockquote \
type="cite" class=""><div class=""><br style="caret-color: rgb(0, 0, 0); font-family: \
Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; \
font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; \
text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; \
float: none; display: inline !important;" class="">[1]<span \
class="Apple-converted-space"> </span></span><a \
href="https://gerrit.ovirt.org/c/ovirt-engine/+/114130" style="font-family: \
Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; \
font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; \
text-indent: 0px; text-transform: none; white-space: normal; widows: auto; \
word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" \
class="">https://gerrit.ovirt.org/c/ovirt-engine/+/114130</a><br style="caret-color: \
rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; \
font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: \
start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: \
0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; \
-webkit-text-stroke-width: 0px; text-decoration: none;" class="">I'd love to get to a \
point when we can automatically gate patches based on OST, but it's going slow…so \
for now it's still manual.<br class=""></blockquote><br style="caret-color: rgb(0, 0, \
0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: \
normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: \
0px; text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; \
float: none; display: inline !important;" class="">Not sure that's enough, but it \
would be a step in the right direction.</span><br style="caret-color: rgb(0, 0, 0); \
font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: \
normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: \
0px; text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; \
float: none; display: inline !important;" class="">Sometimes patches won't break OST \
but are still harmful.</span><br style="caret-color: rgb(0, 0, 0); font-family: \
[Attachment #6 (text/plain)]
_______________________________________________
Devel mailing list -- devel@ovirt.org
To unsubscribe send an email to devel-leave@ovirt.org
Privacy Statement: https://www.ovirt.org/privacy-policy.html
oVirt Code of Conduct: https://www.ovirt.org/community/about/community-guidelines/
List Archives: https://lists.ovirt.org/archives/list/devel@ovirt.org/message/MG4BC3ABRKSXBLPRNQO6LEJEQGS2EFQY/
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic