Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds go module support /v4 #41

Merged
merged 14 commits into from
Aug 3, 2021
Merged

Adds go module support /v4 #41

merged 14 commits into from
Aug 3, 2021

Conversation

mfridman
Copy link
Member

@mfridman mfridman commented Jul 29, 2021

Introduces modules support with with/v4 SIV

  • Update migration guide
  • Update README to ensure import paths are correct referenced

fixes #5 #3 #30

@mfridman mfridman marked this pull request as ready for review July 30, 2021 00:48
@oxisto
Copy link
Collaborator

oxisto commented Jul 30, 2021

To be completely honest, I do not see a big advantage of doing this now. There was an initial wave of people coming over from the old repo to this one now because of the fact that GitHub introduced dependency / security updates for Go, so they were triggered by the dependant security alert of the CVE that was fixed in 3.2.1. Yes of course there were some issues, especially with transitive dependencies, but that was to be expected. I would suggest to let this be settled, take a few weeks to give also major projects to move the dependency over to this project for the 3.2.1. Then we need to fix #40, which could be considered a security fix in some undesired configurations and give people a chance to easily move from 3.2.1 to 3.2.2.

IMHO, creating a v4 release now would just add more chaos instead of actually doing any good. But this is just my 2 cents.

@ripienaar
Copy link

I am inclined to agree. Adding a go mod does not require v4. Though I would love to see the go mod added

@thaJeztah
Copy link
Contributor

adding a go.mod without changing the import path is a breaking change (please don't!) without a go.mod, modules will allow the module (with +incompatible), but after a go.mod is added, it will refuse to use it.

@ripienaar
Copy link

Ah you are right sorry. I forgot the version was already at 3. Then yes we need to bump the version but vote then to wait to get all the inflight stuff done at the same time before tagging 4

@thaJeztah
Copy link
Contributor

Then yes we need to bump the version but vote then to wait to get all the inflight stuff done at the same time before tagging 4

Yes, I think that would make sense;

  1. tag a "final" v3.x release (without go.mod)
  2. optionally: create a v3 branch from last release, to allow backporting security fixes to v3 for some time
  3. move main / master branch to v4, and add go.mod
  4. tag v4.0.0 (no changes in functionality with v3)

Step 4. in the above allows users to migrate from v3 (no modules) to v4 (modules) without risk of pulling in other changes, which reduces friction for users to migrate to v4 (and thus (hopefully) the need for having to maintain the v3 branch for an extended period of time)

It would mean though that if there are plans to introduce breaking changes, a v5.0.0 release would be needed after that, so

5. introduce breaking changes
6. rename module in main branch to github.com/golang-jwt/jwt/v5
7. release as v5.0.0

@mfridman
Copy link
Member Author

To be completely honest, I do not see a big advantage of doing this now. There was an initial wave of people coming over from the old repo to this one now because of the fact that GitHub introduced dependency / security updates for Go, so they were triggered by the dependant security alert of the CVE that was fixed in 3.2.1. Yes of course there were some issues, especially with transitive dependencies, but that was to be expected. I would suggest to let this be settled, take a few weeks to give also major projects to move the dependency over to this project for the 3.2.1. Then we need to fix #40, which could be considered a security fix in some undesired configurations and give people a chance to easily move from 3.2.1 to 3.2.2.

IMHO, creating a v4 release now would just add more chaos instead of actually doing any good. But this is just my 2 cents.

I agree with some of these points, but waiting it just fighting the inventible. Also the earlier we do it, the fewer folks will have to migrate (if they so choose).

It's unfortunate that we're forced into a SIV style import, but this is the landscape of Go modules. My 2 cents is the sooner we can adopt modules the less painful it will be.


@thaJeztah wrt to point 3:

  1. move main / master branch to v4, and add go.mod

Suppose we merge this PR, and then tag a v4.0.0 on main.. is the concern that users on v3.x.y+incompatible will be broken?

If I understood you correctly, you're suggesting we add/maintain v3 (non modules) and v4 as default branch (modules) branches while backporting fixes?

@thaJeztah
Copy link
Contributor

Suppose we merge this PR, and then tag a v4.0.0 on main.. is the concern that users on v3.x.y+incompatible will be broken?

No v3.x.y+incompatible will continue to be working, as long as any v3.x.y+incompatible tag does not contain a go.mod

If I understood you correctly, you're suggesting we add/maintain v3 (non modules) and v4 as default branch (modules) branches while backporting fixes?

I would consider v3 to be marked "deprecated" once v4 is released, and hope no backporting will be needed, except for the rare exception where a critical vulnerability is found before consumers of this module have been able to migrate to v4.
Reason for considering to create a v3 branch is "allow" that exceptional situation to occur, and to prevent "yet another fork" to come to existence. Perhaps it would be ok to clearly mention this in the README; containerd did a good job at that; https://github.com/containerd/containerd/blob/4fdb884644ecdd6a4b4b0d1bd85b8f6ccc1995c6/RELEASES.md#support-horizon
The README could mention that the v3 version, and v3 branch should be considered EOL (or "extended support"), and only critical security patches will be accepted for backporting to that branch (any other backport must be rejected).

It's unfortunate that we're forced into a SIV style import, but this is the landscape of Go modules. My 2 cents is the sooner we can adopt modules the less painful it will be.

I have quite a love/hate relationship with go modules, but in general agree that it would be good to move to modules (v4) as soon as possible.

Under "regular" circumstances, I would say; merge and move, but if #40 is indeed a security fix, I would highly recommend doing the move to v4 after that is merged.

Security releases should have as little friction as possible, and requiring users to accept a breaking change (move to v4, and because of that incompatible with all existing uses of the module), definitely will cause considerable more friction, so if it's possible to postpone that move until after #40 is merged, please do so (again, assuming that's a security fix that users may need to have).

Looking at this PR w.r.t. "pros/cons" to move to modules "ASAP";

  • 👍/👎 This project has no external dependencies, so from that perspective, moving to go modules won't bring benefits to users (modules help to resolve what versions of other dependencies are needed, in this case that doesn't apply)
  • 👍/👎 No more +incompatible in user's go.mod. Nice to have, but other than being a bit of a nuisance to see the +incompatible, no "urgent" fix
  • 👎 Moving to v4 is a breaking change; this is ok, but (ideally) shouldn't prevent users from being able to get a security fix (as outlined above).
  • 👍 Moving to modules "v4" opens up the opportunity to accept breaking changes ("v5"). Despite sometimes being a "pain", the versioned import paths allows users to have multiple versions of the module if needed (they can have both v3 (pre-modules), v4, and v5) in the same codebase.

@oxisto
Copy link
Collaborator

oxisto commented Jul 30, 2021

@mfridman Good points indeed. Ok then let's release 3.2.2; #40 was just merged in and then let's bite the bullet and do a v4 release immediately after that.

@mfridman
Copy link
Member Author

Thanks for the detailed response.

One more 👍 I'd give is to solve transitive dependancy issues esp. when using replace directives (which should not be needed). At that point users should drop replace and instead switch to the /v4, effectively treating this as a separate module altogether.

I pushed a v3 branch https://github.com/golang-jwt/jwt/tree/v3 .. branched off from current main 4bbdd8a which is tagged as v3.2.2. This contains the fix #40

@thaJeztah
Copy link
Contributor

Thank you so much! Have a great weekend 👍

oxisto
oxisto previously approved these changes Jul 31, 2021
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not completely happy with this (not with the PR, but with the whole situation), but I approve :)

This was linked to issues Jul 31, 2021
@oxisto
Copy link
Collaborator

oxisto commented Jul 31, 2021

@mfridman Could you please remove #32 from the list of linked issues (I somehow lack the permissions to do so), because this issue relates to the missing multiple strings in aud, which is going to be fixed in #15, but not in this PR. I will rename that issue (the old name was "v4?" to avoid confusion.

@oxisto oxisto mentioned this pull request Aug 3, 2021
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it.

@oxisto oxisto merged commit 2ebb50f into main Aug 3, 2021
@oxisto oxisto deleted the v4 branch August 3, 2021 13:51
@mfridman
Copy link
Member Author

mfridman commented Aug 3, 2021

@oxisto I won't be able to prepare a release for a few hours (meetings/day job). Any chance yourself or one of the other maintainers can prepare up a v4.0.0 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proper go.mod support Add go module support
4 participants