Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [websocket-dev] [External] : Re: Removing restrictions on direct commits
Hmm. This is really strange. I'm seeing some odd UI behaviour around these settings.
With JSP, where I am trying to make the same change, the UI allows PRs to be required without also requiring at least one reviewer. Some Googling suggests there is a UI bug here. I've tried to work around the UI bug to reconfigure WebSocket to require PRs but not require reviews. I'll need to do some testing to see if it works. Fingers crossed... 

Mark


On 05/11/2021 17:39, Mark Thomas wrote:

On 05/11/2021 16:58, Jan Supol wrote:

Mark,
I saw 3 emails from people saying that they want to keep PRs requirements. I did not see any that supported your wish to remove them. Yet, you step ahead.
I intended to keep the technical control that required PRs but that wasn't possible (as far as I could see) with GitHub's permission scheme. Therefore the options were a) retain the existing technical controls or b) remove the techincal controls and rely on social controls. The latter approach seemed reasonable so I went ahead.
The pool of WebSocket committers providing PRs is small. As far as I can tell, every PR for WebSocket from a committer in the last 12 months has been from me. I think it is likely that social controls will be effective. If they are not, we can always revisit this change.
Social requirements are not enough, since direct push-force to master can be a very dangerous thing,
Can you explain this please. EL has been working this way without any issues I am aware of since creation. What is the risk that you consider dangerous? What can go wrong? This is version control after all. In the worst case, we simply have to revert a commit. 


and hard to trace the origin.
Can you expand on this? We know who made the commit and the commit message should provide sufficient explanation. Anything substantive will be via PR anyway. We are only talking about relatively trivial commits (like the ones in #379 and #380).
What do you see as the benefits of #379 and #380 being via PR rather than a direct commit? 


I expressed what I think about it already.
You have expressed the what but not the why and I'd like to understand the why because, at the moment, I don't understand your position.
Your idea of good faith is not good enough; everyone can make a mistake and the PRs are to eliminate them.
PRs don't eliminate mistakes. The handful of copyright fixes I've just applied demonstrate that.
In my experience, across a wide range of OSS projects, using technical controls rather than social controls has a stifling effect on the community. Given the relatively low levels of activity in the project I view anything the reduces friction as a good thing.
There are lots of examples of us using PRs to discuss potential changes. #356 is a good example where it looks like the proposal is going to be dropped entirely in favour of a better option. 

What mistakes are you expecting PRs to eliminate?

Mark




Thanks,
Jan
------------------------------------------------------------------------
*From:* websocket-dev <websocket-dev-bounces@xxxxxxxxxxx> on behalf of Mark Thomas <markt@xxxxxxxxxx> 
*Sent:* Friday, November 5, 2021 5:48 PM
*To:* websocket developer discussions <websocket-dev@xxxxxxxxxxx>
*Subject:* Re: [websocket-dev] [External] : Re: Removing restrictions on direct commits 
All,

Now I have admin access it appears that if PRs are required then so are
reviews. We can't require PRs without the review. Therefore I am going
to remove the PR requirement.

The social requirement that substantive changes must be via PR and must
allow time for community review remains.

I'll try and make all my changes via PR but a few trivial fixes may end
up going directly to master.

Mark


On 04/11/2021 23:18, Ed Bratt wrote:
When reviewing changes further down the road, I find it easier to review via PRs. I understand you can see the direct commits as well, but in my perspective, these are harder to follow and unwind.
My preference -- and this is mostly as a reader -- is that projects adopt a convention to use PRs, even for small changes -- even if the submitter is just going to turn right around and hit the merge button. 

My opinion, that's all.

-- Ed

On 11/4/2021 1:48 PM, Mark Thomas wrote:
As a first step, I have opened an issue to make the project leads admins - as we should according to the Eclipse Handbook. With admin karma, we should then be able to make changes to the review requirements. 

Mark


On 02/11/2021 16:29, Mark Thomas 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
_______________________________________________
websocket-dev mailing list
websocket-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!ZXaPJ_2lrxntT4crrTbge_DgyBpJUYhh2hOFgetAtSlIXV4Ra_qRKksqY9T8pi0$
<https://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!ZXaPJ_2lrxntT4crrTbge_DgyBpJUYhh2hOFgetAtSlIXV4Ra_qRKksqY9T8pi0$> 



_______________________________________________
websocket-dev mailing list
websocket-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!ZXaPJ_2lrxntT4crrTbge_DgyBpJUYhh2hOFgetAtSlIXV4Ra_qRKksqY9T8pi0$
<https://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!ZXaPJ_2lrxntT4crrTbge_DgyBpJUYhh2hOFgetAtSlIXV4Ra_qRKksqY9T8pi0$> 




_______________________________________________
websocket-dev mailing list
websocket-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!aE6EGNbfFFvU6s1M10dA24CFRYmgf2yFA7YivHmg-GSukZ6Gl-IvOnFZReOPjQRQ$ <https://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!aE6EGNbfFFvU6s1M10dA24CFRYmgf2yFA7YivHmg-GSukZ6Gl-IvOnFZReOPjQRQ$> 

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



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

Back to the top