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

Tidy: Complete TrustedProxies feature #2887

Merged
merged 4 commits into from Oct 6, 2021
Merged

Conversation

Bisstocuz
Copy link
Contributor

@Bisstocuz Bisstocuz commented Sep 29, 2021

Background

Related issues: #2814 #2697 #2819
Related pull requests: #2832 #2692

About #2692 :

This fix has 2 problems:

  1. it's not backward-compatible
  2. is doesn't update related documents

#2692 (comment) by @leafduo

What I do

So I did these things below in this PR:

  1. Update related documents.
  2. Make Engine.TrustedProxies private, force people to use Engine.SetTrustedProxies() to set proxies.
  3. Added proxies safety check and notices in Run, RunTLS, etc.
  4. Completed related unit tests.

This PR should be published with #2692 .

@Bisstocuz Bisstocuz changed the title Refactor: Complete TrustedProxies feature Tidy: Complete TrustedProxies feature Sep 29, 2021
@Bisstocuz
Copy link
Contributor Author

Also hope you guys to help #2844 with code review.

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #2887 (5b8833e) into master (1a2bc0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2887   +/-   ##
=======================================
  Coverage   98.75%   98.76%           
=======================================
  Files          41       41           
  Lines        3063     3065    +2     
=======================================
+ Hits         3025     3027    +2     
  Misses         26       26           
  Partials       12       12           
Flag Coverage Δ
go-1.13 98.76% <100.00%> (+<0.01%) ⬆️
go-1.14 98.59% <100.00%> (+<0.01%) ⬆️
go-1.15 98.59% <100.00%> (+<0.01%) ⬆️
go-1.16 98.59% <100.00%> (+<0.01%) ⬆️
go-1.17 98.49% <100.00%> (+<0.01%) ⬆️
macos-latest 98.76% <100.00%> (+<0.01%) ⬆️
nomsgpack 98.74% <100.00%> (+<0.01%) ⬆️
ubuntu-latest 98.76% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
context.go 97.98% <ø> (ø)
gin.go 99.10% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a2bc0e...5b8833e. Read the comment docs.

@Bisstocuz
Copy link
Contributor Author

Bisstocuz commented Sep 30, 2021

Hello, help with code review please.
@zihengCat @leafduo @AlbinoGeek @menduo

@thinkerou thinkerou added this to the v1.8 milestone Sep 30, 2021
@appleboy appleboy modified the milestones: v1.8, v1.7.5 Oct 1, 2021
appleboy
appleboy previously approved these changes Oct 1, 2021
@appleboy
Copy link
Member

appleboy commented Oct 1, 2021

This PR should be merge to v1.7.5

cc @thinkerou

@Bisstocuz
Copy link
Contributor Author

@appleboy
Excuse me, I fixed some typo so you need to approve again plz.

Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

lgtm

@Bisstocuz
Copy link
Contributor Author

@appleboy Hi, this PR can be merged.

@appleboy appleboy merged commit 3918132 into gin-gonic:master Oct 6, 2021
24 checks passed
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021
@ItalyPaleAle
Copy link
Contributor

@thinkerou @appleboy
Please note that this:

Make Engine.TrustedProxies private, force people to use Engine.SetTrustedProxies() to set proxies.

Was a breaking change, and should not have been merged in a patch version. It did break people's code.

See https://go.dev/blog/v2-go-modules :

For projects which are declared stable — at major version v1 or higher — breaking changes must be done in a new major version.

daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants