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 
On 09/11/2021 23:48, Jan Supol wrote:

Mark,
I realize that from the very majority you drive the WebSocket Spec forward, and the WebSocket community appreciates that.
I understand that the WebSocket committer number is lower, and you wanted to be able to make small fast changes. But the direct commits are not the best way to solve it. With force push, it is not only possible to add commits, but it is also possible to remove commits. The PRs prevent that.
Anyway, as for why, I'd assume it would be a good reason that committers did not want direct PRs.
I agree force push is horrible but requiring all changes to be via PRs is only one way of preventing that. GitHub's branch protection can block force pushes explicitly.
It appears there was some UI strangeness going on with GitHub. The short version is I was able to hack around the UI strangeness I was seeing and WebSocket is currently configured to require PRs but those PRs do not require an approving review to be committed. I believe this addresses your concern and removes the majority of the friction I was concerned about.
I'm currently intending to leave these settings alone for a while and see how things go. If we identify problems then we can revisit the branch protection settings as required. 

Mark



Thanks,
Jan

------------------------------------------------------------------------
*From:* websocket-dev <websocket-dev-bounces@xxxxxxxxxxx> on behalf of Mark Thomas <markt@xxxxxxxxxx> 
*Sent:* Friday, November 5, 2021 6:39 PM
*To:* websocket-dev@xxxxxxxxxxx <websocket-dev@xxxxxxxxxxx>
*Subject:* Re: [websocket-dev] [External] : Re: Removing restrictions on direct commits 
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$>
<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$>
<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$>
<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://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!coubJkvEI4EKK6SprL0sNT0hB8mcN2rndT2-DSe-Aqf-6LdIj183lbGj7uSSOm4z$
<https://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!coubJkvEI4EKK6SprL0sNT0hB8mcN2rndT2-DSe-Aqf-6LdIj183lbGj7uSSOm4z$> 



_______________________________________________
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!coubJkvEI4EKK6SprL0sNT0hB8mcN2rndT2-DSe-Aqf-6LdIj183lbGj7uSSOm4z$ <https://urldefense.com/v3/__https://www.eclipse.org/mailman/listinfo/websocket-dev__;!!ACWV5N9M2RV99hQ!coubJkvEI4EKK6SprL0sNT0hB8mcN2rndT2-DSe-Aqf-6LdIj183lbGj7uSSOm4z$> 

_______________________________________________
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