gorilla/websocket

Restore trust in the project

Opened this issue · 18 comments

Is there an existing feature request for this?

  • I have searched the existing feature requests

Is your feature request related to a problem? Please describe.

Unreviewed and harmful changes were committed in 666c197 (the 666 commit). The attempt to undo those changes is incomplete and left go test failing.

Describe the solution that you would like.

Submit a PR to revert back to the last good version of the repository, 8983b96.

Merge or discard changes made since that commit as follows:

8039329 - Raise an issue to update other examples to return from HTTP handler as described in the PR. Delete this example because it does not show anything unique.

666c197 - Discard. This is the commit damaged the project.

78c3487 - ?
ac0789b - ?

aa97606 - Merge

b6a0d77 - Merge

629990d - Merge

6f5d213 - Discard. Fixes bug introduced in discarded commit 666c197.

286c896 - Merge

01b0aae - Discard. Because this package does not define the default buffer sizes on the server, there's no guarantee that the comment is accurate. If an application programmer does care about the size, then they should specify a size instead of relying on a default.

dcea2f0 - Discard. Fixes bug introduced in discarded commit 666c197.

871f6bb - Merge

4a5e66f - Discard. The change does not significantly decrease the time to run the test.

1e975a0 - Merge.

0f0acef - Merge.

9a21405 - Discard. Fixes bug introduced in discarded commit 666c197.

4965080 - Discard. Fixes bug introduced in discarded commit 666c197.

7d5b8cc - Discard. Do not write to application's stderr.

cf50a3e - Discard. Reverts previous commit.

d15aba1 - Merge

d08ee1a and 3168614 - Merge

0cfb2ca and d293aa5 - Merge, but fix per #910.

695e909 - Discard. Fixes bug introduced in discarded commit 666c197.

b2a86a1 - Merge

58af150 - Merge

09a6bab - Discard. Fixes bug introduced in discarded commit 666c197.

b2c246b, 1bddf2e - Discard. This is an incomplete attempt to revert 8039329.

5fc9a59 - Merge.

eb8de5f - Merge.

Describe alternatives you have considered.

Forking the project.

Anything else?

My proposal is very feasible. See https://github.com/tebuka/websocket where I cherry picked the useful commits from this repository.

Hey @tebuka , we just cut a new release v1.5.3 which reverts back to 931041c . We'll be stepping through the rest of the commits and trying to reapply in a way that makes sense - the steps you shared make a great guideline for how to proceed and I greatly appreciate you taking the time to look through this.

Take a look at the Tebuka WebSocket fork. I cherry-picked the good changes since 76ecc29. The issue tebuka/websocket/issues/1 is an improved version of what I wrote above.

The Tebuka repo contains additional improvements beyond the cherry-picked commits.

You are welcome to pick commits from Tebuka WebSocket.

I created the Tebuka fork because I lost all confidence in the Gorilla maintainers. I have more work to do before announcing Tebuka to the world. After landing a PR in progress, I'll pause that work and wait to see what happens here.

Thanks for sharing @canelohill, and yeah that's the beauty of open source. We'll try to restore the communities trust. It's a little slower than we'd like cause we all have full time jobs and the gorilla org actually has a lot more repos that we help to maintain.

I thought I'd help out because are are all busy with your jobs. I submitted PR #930, PR #931, PR #932 and PR #933 to get things started.

I suggest using rebase and merge on these PRs to retain original author information and linear history.

You're amazing thank you @canelohill , I'll endeavour to take a look very soon.

jech commented

Version 1.5.3 has a commit called "Reverts to v1.5.0". However, v1.5.3 is not identical to v1.5.0:

$ git diff v1.5.0..v1.5.3 --stat
 .circleci/config.yml  |  6 +++---
 README.md             |  6 ------
 client.go             | 18 +++++++++++++++---
 client_server_test.go | 35 +++++++++++++++++++++++++++++++++++
 conn.go               |  8 ++++++++
 conn_test.go          | 12 +++++++++++-
 server.go             |  4 ++--
 server_test.go        |  2 +-
 util.go               | 15 +++++++++++++++
 util_test.go          | 19 +++++++++++++++++++
 10 files changed, 109 insertions(+), 16 deletions(-)

Please publish a commit that is identical to v1.5.0, so that those of us who review changes carefully can verify that you're not hiding anything. After you've done that, please include a commit that adds the following to go.mod:

retract (
     v1.5.1
     v1.5.2
     v1.5.3
)

and tag a new version.

Trust has been broken by your careless actions, and you now need to do things properly in order to reestablish it.

I’m thankful that we have folks that care so much about this project like yourself @jech.

In simple reply to your demand the answer is no. If you require v1.5.0 then the tagged release is available for you to use.

I’m not sure if you took the time to carefully look at the history of v1.5.3 in GitHub issues before you took the time to write your demand, but I’ll summarise here: We owned up to making a mistake by saying that the merged + squashed commit “reverted to 1.5.0”, but we were transparent in release notes and issues that the mistake was made in naming it that way.

There are several others who deeply contribute to this project, who are now able to trust this project — some who are in this thread.

Now that it’s clear about the commit being misnamed (and for transparency sake, the other contributor named it that on my suggestion, so it’s entirely my fault that this confusion exists) you should be able to look over the history with fresh eyes.

@jech Here's the difference between 1.5.0 and the "Reverts to 1.5.0" commit: v1.5.0...ce903f6 . These differences are the result of commits in #839, #798, #791, #788, #773, and #752. The first is a change to the readme file. The remaining PRs were reviewed by the previous maintainer. The difference is small enough to review by hand.

I think the code is in good shape at the moment, but I am reserving judgment on the future of the project. The maintainers are headed in the right direction now, but I need to see more of a good track record to have confidence in their stewardship of the project.

jech commented

Here's the difference between 1.5.0 and the "Reverts to 1.5.0" commit: v1.5.0...ce903f6 . These differences...

The fact that we've needed to do this work ourselves is the result of the maintainers' continued unwillingness to provide a reviewable git log with meaningful commit messages. Unfortunately, in the wake of the libxz disaster, no responsible project can depend on unreviewable code dumps.

jech commented

@jaitaiwan:

In simple reply to your demand the answer is no.

Thank you for your reply. It is therefore the official policy of this project to provide unreviewable code dumps rather than a proper git history. Unfortunately, events such as the libxz disaster show that no responsible project should rely on unreviewable code dumps.

I will therefore be forking v1.5.0, which I was trying to avoid doing in order to avoid fragmenting the community. Since Galene will depend on the fork, the fork will be maintained for the foreseeable future. Please do not close this bug in the meantime.

Good luck! I’ll be following your fork with interest to see how you manage to prevent your consumers the laborious task of running a diff.

Ill just leave this little quote here:

There are only two hard things in Computer Science: cache invalidation and naming things.
-- Phil Karlton

@jech Here's an easier way to look at it. There are no differences between 931041c and ce903f6 (view diff here). The changes outside that range are all reviewable.

Would it have been better to remove the mess from the history by resetting head to 931041c and moving on from there? I don't know the answer to that because I am not expert in proper maintenance of git history. I do know that a simple script can move the changes from the current history to a new history starting at 931041c (or v1.5.0 for that matter).

You may want to get in touch with FZambia. They are also maintaining a fork of this project.

jech commented

You misattributed the [...] quote to me.

Sorry for that. Fixed.

jech commented

Good luck! I’ll be following your fork with interest to see how you manage to prevent your consumers...

I think there's a misunderstanding.

I'am forking gorilla/websocket in order to have a source to depend on in my own projects. This is unfortunate, since by forcing every project to maintain their own copy of gorilla/websocket, you are fragmenting the community.

Hey @jech - forking and maintaining your own copy of an OSS library to fit your personal needs is not uncommon for individuals to do and shows the true beauty of open source. We as the maintainers are comfortable with where the project sits now and will be going forward from this point. In the spirit of OSS, we hope that you choose to send any bugfixes or improvements back upstream. Best of luck!

jech commented

forking and maintaining your own copy of an OSS library to fit your personal needs is not uncommon

Please don't be flippant. The project suited my needs perfectly until your colleagues broke it. This is extra work for me and others who find themselves in the same situation that is entirely due to the incompetence and bad will of some RedHat engineers.

As you justly note, it also fragments the community, and makes it less likely that bug fixes end at the right place.

@jech Let me summarise your position and throw my hands up in metaphorical exacerbation. I'll not speak for other maintainers here, only myself.

You said that you:

that those of us who no longer trust you

So we have here that you have started interacting with a default position of hostility.

Trust has been broken by your careless actions
This is a completely fair comment, but the maintainers already admitted fault and worked with the community to release 1.5.3 - a non-broken but forward-moving release of the library.

You asked:

Please publish a commit that is identical to v1.5.0,
To wit I already noted that 1.5.0 as a release is pointed to a known good commit so you already have that.

You made the assertion that:

do things properly in order to reestablish it

Which is to do things according to your apparently superior standard.

So with simple, fool-proof tools literally built into github or git itself... you can see the history of the repository, our mistake included (which by the way says A LOT more about our transparency and trustworthiness than either redoing history or retracting yet another release just for the sake of one user who can't be bothered to go through an incredibly tiny diff [10 files!]) and yet you insist on having a whinge about how the maintainers aren't doing things the way that you alone feel is worthy enough of your almighty blessing?

You then comment to say:

This is extra work for me and others who find themselves in the same situation that is entirely due to the incompetence

What a horrible injustice you have had to endure! We are all human, and we all make mistakes. If you cannot have grace for folks who are making a genuine effort to improve something, who are honest and own up to their mistakes, then what sort of community do you expect to build around your own work when you inevitably make a mistake yourself?

bad will of some RedHat engineers

You have literally 0 evidence to prove any bad will, and RedHat is not remotely involved in this.

Ridiculous. Learn to be moderate or stop wasting the time of folks who maintain software entirely of their own free will and precious time. Until then, please move on. I sincerely hope you come to your senses and apologise for your lack of grace and unsubstantiated black-and-white position, and that you don't have the misfortune of experiencing this level of nonsense with your own projects.

My apologies to others who end up having to read this or endure this thread. I'm going to close it out for now.