Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [servlet-dev] Removing restrictions on direct commits 
On 03/11/2021 01:48, Stuart Douglas wrote:
On Wed, 3 Nov 2021 at 11:02, Greg Wilkins <gregw@xxxxxxxxxxx <mailto:gregw@xxxxxxxxxxx>> wrote: 


    I kind of like having it, as we should very rarely be in a situation
    where a PR must be committed ASAP.
    If a PR really needs to be committed ASAP, then I think project
    admins have the power to override on a case by case basis anyway.


None of us are admins though AFAIK.


Correct.
Personally I would prefer that Mark be made an admin and the 'Include Administrators' box be unticked. It would mean that Mark could merge if he really needs something for the release, but it is a big red button with a warning.
That would address my immediate concern regarding being able to address release issues in a timely manner. The main thing I don't like about this is that it gives me karma other committers don't have. 


    As a committer to many projects, I have often self determined that
    my changes were ready to be committed... only to be surprised by
some silly omission, accidental inclusion or plain stupid mistake.     We are all fallible and even if not, then does the extra few 
    hours required to wait for a review really hurt?

    cheers

    P.S. If you do turn it off, I'm still going to pester others for
    review of any of my PRs, as I for one have broken too many builds to
    have any delusions of infallibility.
Oh indeed. I am by no means infallible either. However, my experience of the last few years where EL does not have this restriction is that always requiring it adds unnecessary friction to the project. I am confident that all the committers can make good decisions on when to just commit something and when a PR + discussion is required. Even if they do get it wrong (very unusual in my experience) we can always revert things if necessary.
The process should remain the same either way, and we should definitely keep the 'Require a pull request before merging' protection even if you are allowed to merge your own PR's, just to prevent accidental pushes.
I'm not convinced (working on multiple projects where PRs are not required) that accidental pushes are that much of a problem but I could live with that.
I looks like my initial proposal goes further than the community is comfortable with but that the community would accept some degree of relaxation. With that in mind I'm amending my proposal to the following.
Remove the GitHub enforced requirement that all PRs must be reviewed. All changes will still be via PR. Committers will be expected to use their judgement as to how long to allow for feedback before merging a PR. 

Mark




Stuart





    On Wed, 3 Nov 2021 at 03:30, Mark Thomas <markt@xxxxxxxxxx
    <mailto:markt@xxxxxxxxxx>> wrote:

        All,

        In approx 24 hours time I intend to request that the branch
        restrictions
        that prevent committers committing directly to the master branch
        and
        those that require every PR to be reviewed before merge are removed.

        My reasoning is as follows:
        - I have seen the benefits of these restrictions not being
        present in EL
        - I'm expecting a number of non-substantive changes will be
        required to
            successfully complete the release process and PR + review
        for all of
            them will significantly slow us down
        - Committers are perfectly capable of determining which changes
        need a
            PR and review and which can be made directly - and if they
        get it
            wrong changes can easily be reverted

        I was intending to propose this change after the Jakarta 10
        release but
        on reflection, I think the sooner, the better.

        Thoughts? Comments? Objections?

        Mark
        _______________________________________________
        servlet-dev mailing list
        servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
        To unsubscribe from this list, visit
        https://www.eclipse.org/mailman/listinfo/servlet-dev
        <https://www.eclipse.org/mailman/listinfo/servlet-dev>
-- Greg Wilkins <gregw@xxxxxxxxxxx <mailto:gregw@xxxxxxxxxxx>> CTO 
    http://webtide.com <http://webtide.com>
    _______________________________________________
    servlet-dev mailing list
    servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
    To unsubscribe from this list, visit
    https://www.eclipse.org/mailman/listinfo/servlet-dev
    <https://www.eclipse.org/mailman/listinfo/servlet-dev>


_______________________________________________
servlet-dev mailing list
servlet-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/servlet-dev

Back to the top