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

bump golang deps #31422

Merged
merged 43 commits into from
Jun 24, 2024
Merged

bump golang deps #31422

merged 43 commits into from
Jun 24, 2024

Conversation

techknowlogick
Copy link
Member

No description provided.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 19, 2024
}

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)))
Copy link
Member

Choose a reason for hiding this comment

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

ldap:// here

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@silverwind silverwind Jun 19, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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://host:port syntax.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@silverwind silverwind self-requested a review June 19, 2024 17:32
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 19, 2024
@github-actions github-actions bot removed the modifies/api This PR adds API routes or modifies them label Jun 19, 2024
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

LGTM apart from the open discussion

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 19, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 20, 2024
@silverwind
Copy link
Member

silverwind commented Jun 24, 2024

Let's merge and revisit the go-ldap change later in a dedicated PR.

@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 24, 2024
@silverwind silverwind enabled auto-merge (squash) June 24, 2024 12:59
@silverwind silverwind merged commit a4899ff into go-gitea:main Jun 24, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jun 24, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 24, 2024
@techknowlogick techknowlogick deleted the bump-deps-20240619 branch June 24, 2024 13:56
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 25, 2024
* 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)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/dependencies modifies/go Pull requests that update Go code modifies/internal size/L Denotes a PR that changes 100-499 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants