geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jinmei Liao <jil...@pivotal.io>
Subject Re: [DISCUSS] is overriding a PR check ever justified?
Date Thu, 31 Oct 2019 21:37:02 GMT
I am not sure whether this is related to this topic or not, but recently I
had to revert one of my commit, before I could just do a revert and push to
develop, but now that is blocked. I had to file a PR to revert a change
that's causing the pipeline to be red. My question is: do we still need to
follow the same process (waiting for one review approval) to revert a
commit?

On Thu, Oct 31, 2019 at 10:17 AM Udo Kohlmeyer <udo@apache.com> wrote:

> You are correct... Break glass is a sign that whatever needed to be
> done, was not going to work using the prescribed approach..
>
> I see this "break glass" as a special handshake or someone with more
> "authority" needs to be agree with this... but given there is not
> "someone with more authority" in Apache... this would have to be
> consensus between either committers or PMC members.
>
> --Udo
>
> On 10/31/19 9:58 AM, Mark Hanson wrote:
> > -1 for "Break glass" approach. Needing a break glass approach is a sign.
> I wonder how hard that would be to make exist. I think our break glass
> approach is that we have someone with the power disable the restrictions in
> Github for the window that we must and then we put it back.
> >
> > Thanks,
> > Mark
> >
> >> On Oct 31, 2019, at 9:26 AM, Benjamin Ross <bross@pivotal.io> wrote:
> >>
> >> I would agree with udo, +1 to having an emergency 'break glass' override
> >> but -1 to having the ability to do it routinely or for convenience.
> >>
> >> The main use case in my mind is for infrastructure related issues that
> >> block a PR behind unrelated changes which can be really frustrating and
> >> inconvenient (StressNewTest is a big culprit here) but I'm hoping that
> if
> >> constant issues with this arise it will lead to fixing the offending
> >> infrastructure, whether that means changing test itself or the
> architecture
> >> in which it runs, to make our pipelines more reliable. If we smooth over
> >> PR's that run into issues every time Stress Tests break a test which
> only
> >> had logging changes (or similar situations) then there's no incentive
> for
> >> us to improve the Stress Tests job.
> >>
> >> Having said all that, being completely without the ability to perform an
> >> emergency override if everything goes sideways and the only way to fix
> it
> >> is to push a commit which can't get a green pipeline seems like a pretty
> >> good idea to me as long as we all agree on what circumstances qualify
> as an
> >> emergency.
> >>
> >> On Thu, Oct 31, 2019 at 2:43 AM Ju@N <jujoramos@gmail.com> wrote:
> >>
> >>> -1 for allowing overrides.
> >>> If there's an edge case on which it's required, then we could use it
> at the
> >>> very last resources *if and only if* it's been discussed and approved
> >>> through the dev list for that particular case.
> >>> Cheers.
> >>>
> >>>
> >>> On Wed, Oct 30, 2019 at 11:35 PM Robert Houghton <rhoughton@pivotal.io
> >
> >>> wrote:
> >>>
> >>>> Any committer has the 'status' permission on apache/geode.git. Some
> API
> >>>> tokens have it as well. Its curl + github API reasoning to do this.
> >>>>
> >>>> On Wed, Oct 30, 2019 at 2:02 PM Jason Huynh <jhuynh@pivotal.io>
> wrote:
> >>>>
> >>>>> If we are going to allow overrides, then maybe what Owen is
> describing
> >>>>> should occur.  Make a request on the dev list and explain the
> >>> reasoning.
> >>>>> I don't think this has been done and a few have already been
> >>> overridden.
> >>>>> Also who has the capability to override and knows how.  How is that
> >>>>> determined?
> >>>>>
> >>>>> On Wed, Oct 30, 2019 at 1:59 PM Owen Nichols <onichols@pivotal.io>
> >>>> wrote:
> >>>>>>> How do you override a check, anyway?
> >>>>>> Much like asking for jira permissions, wiki permissions, etc,
just
> >>> ask
> >>>> on
> >>>>>> the dev list ;)
> >>>>>>
> >>>>>> Presumably this type of request would be made as a “last resort”
> >>>>> following
> >>>>>> a dev list discussion wherein all other reasonable options had
been
> >>>>>> exhausted (reworking or splitting up the PR, pushing empty commits,
> >>>>>> rebasing the PR, etc)
> >>>>>>
> >>>>>>> On Oct 30, 2019, at 1:42 PM, Dan Smith <dsmith@pivotal.io>
wrote:
> >>>>>>>
> >>>>>>> +1 for allowing overrides. I think we should avoid backing
> >>> ourselves
> >>>>>> into a
> >>>>>>> corner where we can't get anything into develop without
talking to
> >>>>> apache
> >>>>>>> infra. Some infrastructure things we can't even fix without
> >>> pushing a
> >>>>>>> change develop!
> >>>>>>>
> >>>>>>> How do you override a check, anyway?
> >>>>>>>
> >>>>>>> -Dan
> >>>>>>>
> >>>>>>> On Wed, Oct 30, 2019 at 12:58 PM Donal Evans <doevans@pivotal.io>
> >>>>> wrote:
> >>>>>>>> -1 to overriding from me.
> >>>>>>>>
> >>>>>>>> The question I have here is what's the rush? Is anything
ever so
> >>>>>>>> time-sensitive that you can't even wait the 15 minutes
it takes
> >>> for
> >>>> it
> >>>>>> to
> >>>>>>>> build and run unit tests? If some infrastructure problem
is
> >>>> preventing
> >>>>>>>> builds or tests from completing then that should be
fixed before
> >>> any
> >>>>> new
> >>>>>>>> changes are added, otherwise what's the point in even
having the
> >>> pre
> >>>>>>>> check-in process?
> >>>>>>>>
> >>>>>>>> -Donal
> >>>>>>>>
> >>>>>>>> On Wed, Oct 30, 2019 at 11:44 AM Nabarun Nag <nnag@apache.org>
> >>>> wrote:
> >>>>>>>>> @Aaron
> >>>>>>>>> It's okay to wait for at least the build, and unit
tests to
> >>>> complete,
> >>>>>> to
> >>>>>>>>> cover all the bases. [There may have been commits
in between
> >>> which
> >>>>> may
> >>>>>>>>> result in failure because of the revert]  And it's
not hard to
> >>> get
> >>>> a
> >>>>> PR
> >>>>>>>>> approval.
> >>>>>>>>>
> >>>>>>>>> -1 on overriding. If the infrastructure is down,
which is the
> >>> test
> >>>>>>>>> framework designed to ensure that we are not checking
in unwanted
> >>>>>> changes
> >>>>>>>>> into Apache Geode, wait for the infrastructure to
be up, get your
> >>>>>> changes
> >>>>>>>>> verified, get the review from a fellow committer
and then
> >>> check-in
> >>>>> your
> >>>>>>>>> changes.
> >>>>>>>>>
> >>>>>>>>> I still don't understand why will anyone not wait
for unit tests
> >>>> and
> >>>>>>>> build
> >>>>>>>>> to be successful.
> >>>>>>>>>
> >>>>>>>>> Regards
> >>>>>>>>> Nabarun Nag
> >>>>>>>>>
> >>>>>>>>> On Wed, Oct 30, 2019 at 11:32 AM Aaron Lindsey <
> >>>> alindsey@pivotal.io>
> >>>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>> One case when it might be acceptable to overrule
a PR check is
> >>>>>>>> reverting
> >>>>>>>>> a
> >>>>>>>>>> commit. Before the branch protection was enabled,
a committer
> >>>> could
> >>>>>>>>> revert
> >>>>>>>>>> a commit without a PR. Now that PRs are mandatory,
we have to
> >>> wait
> >>>>> for
> >>>>>>>>> the
> >>>>>>>>>> checks to run in order to revert a commit. Usually
we are
> >>>> reverting
> >>>>> a
> >>>>>>>>>> commit because it's causing problems, so I think
overruling the
> >>> PR
> >>>>>>>> checks
> >>>>>>>>>> may be acceptable in that case.
> >>>>>>>>>>
> >>>>>>>>>> - Aaron
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On Wed, Oct 30, 2019 at 11:11 AM Owen Nichols
<
> >>>> onichols@pivotal.io>
> >>>>>>>>> wrote:
> >>>>>>>>>>> Our new branch-protection rules can sometimes
lead to
> >>> unexpected
> >>>>>>>>>> obstacles
> >>>>>>>>>>> when infrastructure issues impede the intended
process.  Should
> >>>> we
> >>>>>>>>>> discuss
> >>>>>>>>>>> such cases as they come up, and should overruling
the result
> >>> of a
> >>>>> PR
> >>>>>>>>>> check
> >>>>>>>>>>> ever be an option on the table?
> >>>>>>>>>>>
> >>>>>>>>>>> -Owen
> >>>>>>
> >>>
> >>> --
> >>> Ju@N
> >>>
>


-- 
Cheers

Jinmei

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message