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

chore: update golang-jwt/jwt and other modules #1261

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

andreaciri
Copy link

This PR updates all go modules to the latest version.

The main changes are related to golang-jwt/jwt v5, for which I followed the migration guide.

The only breaking change I'm introducing in go kit is the rename of StandardClaimsFactory() into RegisteredClaimsFactory() in order to keep consistency with the underlying naming.

Comment on lines -82 to +85
// StandardClaimsFactory is a ClaimsFactory that returns
// an empty jwt.StandardClaims.
func StandardClaimsFactory() jwt.Claims {
return &jwt.StandardClaims{}
// RegisteredClaimsFactory is a ClaimsFactory that returns
// an empty jwt.RegisteredClaims.
func RegisteredClaimsFactory() jwt.Claims {
return &jwt.RegisteredClaims{}
Copy link
Author

Choose a reason for hiding this comment

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

In case naming consistency is not so critical, we can consider to keep StandardClaimsFactory so to avoid a breaking change in go kit.
From my understanding jwt.RegisteredClaims{} is mostly backwards compatible with jwt.StandardClaims{}.

Any feedback is appreciated

Copy link
Member

Choose a reason for hiding this comment

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

Changing public API identifiers like this should probably be avoided if at all possible.

@peterbourgon
Copy link
Member

A major version bump signals backwards-incompatible changes, essentially an entirely different module than the previous major version. If you want to use jwt/v5 then I'd like to see it either as a new Go kit "jwt2" package, or a pretty comprehensive test suite to ensure that "mostly backwards compatible" actually means "fully backwards compatible" in terms of the API that Go kit offers to its consumers.

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.

2 participants