bump golang deps #31422
bump golang deps #31422
Conversation
github-actions
bot
added
the
modifies/api
GiteaBot
added
lgtm/need 1
| } | ||
|
|
||
| conn, err := ldap.Dial("tcp", net.JoinHostPort(source.Host, strconv.Itoa(source.Port))) | ||
| conn, err := ldap.DialURL(net.JoinHostPort(source.Host, strconv.Itoa(source.Port))) |
There was a problem hiding this comment.
I added it in, although I think by adding it in, it may alter the behaviour. As looking at the diff for the ldap import changes, previously it handled the missing protocol the same way as it does now.
There was a problem hiding this comment.
What triggered this change? A deprecation? DialURL sounds like it should only be passed URLs, but I guess better to look at the source code of those functions because the docs are lacking.
There was a problem hiding this comment.
Here's the diff: go-ldap/ldap@v3.4.6...v3.4.8#diff-4f427d2b022907c552328e63f137561f6de92396d7a6e8f6c2ea1bcf0db52654
Looks like those functions were already deprecated, it's just a change in comments allowed it to be picked up by our linter, and that the functions we used didn't modify the protocol at all. So I will remove my additions of hardcoding a protocol.
There was a problem hiding this comment.
I think it's wrong to exclude the scheme. A URL without a scheme is not a URL.
Have a look at DialURL and specifically this line, it shows TLS is only enable when u.Scheme === "ldap" which can not be true with the current argument passed to DialURL.
Also take a look at this example. One is simply not supposed to omit scheme in DialURL.
There was a problem hiding this comment.
fair. I'm not sure I can run through a full testing of LDAP right now, so what if I revert the ldap changes, and revisit it in an upcoming PR?
There was a problem hiding this comment.
Seems like a harmless change to do imho. Actual deprecation came in go-ldap/ldap@2517798 and in their test suite, you can also see the change from host:port to scheme:https://host:port syntax.
There was a problem hiding this comment.
If you really must, add a //nolint for it, but I'd much prefer to do the change and run the ldap tests which should be running as part of CI anyways.
GiteaBot
added
lgtm/need 2
github-actions
bot
removed
the
modifies/api
GiteaBot
added
lgtm/need 1
GiteaBot
added
lgtm/done
|
Let's merge and revisit the |
silverwind
added
the
reviewed/wait-merge
GiteaBot
removed
the
reviewed/wait-merge
* giteaofficial/main: [skip ci] Updated translations via Crowdin Fix poor table column width due to breaking words (go-gitea#31473) bump golang deps (go-gitea#31422)
No description provided.