-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
refactor: drop old golang-jwt/jwt dependency #39804
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
We are only using a small method that has nothing to do with jwt, it's better to copy the method and drop the dependency.
73d6f56
to
0a3475d
Compare
@@ -307,6 +306,7 @@ require ( | |||
github.com/goccy/go-json v0.10.2 // indirect | |||
github.com/godror/knownpb v0.1.0 // indirect | |||
github.com/golang-jwt/jwt/v4 v4.5.0 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the changes look good, I suggest that since we have github.com/golang-jwt/jwt/v4
as an indirect dependency, why not use the function from there as the code is essentially the same and there is no overhead as far as I know. It is just that github.com/golang-jwt/jwt/v4
becomes a direct dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. Direct and indirect dependencies are not the same and should not be treated the same.
Beats already disables dead code elimination due to problematic use of reflection, adding more direct dependencies only makes the problem worse by increasing binary size for downstream users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is included, right? Indirect are dependencies imported by direct dependencies. So, they are already on the build list. As github.com/golang-jwt/jwt/v4
is already in the build list, I suggested let we make it a direct dependency instead (not v5, but v4).
What do you think?
After #39968 the jwt lib is actually used (which is good because it got rid of another dep) so this PR is not needed anymore. |
Also, as per my discussion with Craig offline, we decided to go with So, dropped one more extra lib (indirectly): github.com/dgrijalva/jwt-go (that too deprecated) |
Proposed commit message
drop old golang-jwt/jwt dependency
We are only using a small method that has nothing to do with jwt, it's better to copy the method and drop the dependency.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Disruptive User Impact
Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs