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

gin.Context with fallback value from gin.Context.Request.Context() #2751

Merged
merged 6 commits into from
Jun 24, 2021
Merged

gin.Context with fallback value from gin.Context.Request.Context() #2751

merged 6 commits into from
Jun 24, 2021

Conversation

wei840222
Copy link
Contributor

@wei840222 wei840222 commented Jun 15, 2021

For many tracing sdk
They will have SpanFromContext function. like:
https://github.com/opentracing/opentracing-go/blob/master/gocontext.go#L26
https://github.com/open-telemetry/opentelemetry-go/blob/main/trace/context.go#L48

And middleware sdk always put trace contenxt to http.Request.Context
https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go#L79

If gin.Context with fallback value from gin.Context.Request.Context(), it can be pass the gin.Context as context.Context to sub function call. It will be usefull.

user, err := h.GetUser(c.Request.Context(), userID)

can be

user, err := h.GetUser(c, userID)

@wei840222 wei840222 closed this Jun 15, 2021
@wei840222 wei840222 reopened this Jun 15, 2021
@wei840222 wei840222 closed this Jun 15, 2021
@wei840222 wei840222 reopened this Jun 15, 2021
@thinkgos
Copy link
Contributor

Good!! I always use c.Request.Context().

@appleboy
Copy link
Member

@wei840222 Please rebase the master branch to trigger unit testing with GitHub Actions.

@appleboy appleboy added this to the v1.8 milestone Jun 24, 2021
@wei840222
Copy link
Contributor Author

hi @appleboy. I've rebased the master, plz check.

@wei840222 wei840222 closed this Jun 24, 2021
youzeliang and others added 5 commits June 24, 2021 10:43
delete more "()"
* ci: add github action workflows

* test: fixed the TestUnixSocket test on windows (#20)

* ci: add github action workflows (#18)

* Remove .travis.yml

* ci: replace GITTER_ROOM_ID and upload coverage every time you go test

* ci: update coverage using codecov/codecov-action@v1

* Merge branch 'master' into github-actions

* repo: replace travis ci to github actions

* ci: add go version 1.16

* fix: go install requires a specific version

* chore(ci): remove go 1.12 support

* chore(ci): remove os windows-latest

Co-authored-by: thinkerou <thinkerou@gmail.com>
Co-authored-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@wei840222 wei840222 reopened this Jun 24, 2021
@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #2751 (e62a357) into master (34ce210) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2751   +/-   ##
=======================================
  Coverage   98.69%   98.70%           
=======================================
  Files          41       41           
  Lines        2072     2079    +7     
=======================================
+ Hits         2045     2052    +7     
  Misses         15       15           
  Partials       12       12           
Impacted Files Coverage Δ
context.go 97.69% <100.00%> (+0.03%) ⬆️
gin.go 99.18% <100.00%> (ø)
tree.go 100.00% <100.00%> (ø)
recovery.go 97.22% <0.00%> (+0.03%) ⬆️

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 34ce210...e62a357. Read the comment docs.

appleboy
appleboy previously approved these changes Jun 24, 2021
@appleboy appleboy requested a review from thinkerou June 24, 2021 03:07
context.go Show resolved Hide resolved
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

@thinkerou thinkerou merged commit 7834a03 into gin-gonic:master Jun 24, 2021
@thinkgos
Copy link
Contributor

thinkgos commented Jun 24, 2021

@wei840222 @appleboy @thinkerou This may be break change, I always use c.Request.Context(), It can be cancel or timeout when I use middleware. We should Done() and Deadline(),Err() API fallback c.Request.Done(), c.Request.Deadline(),c.Request.Err() too ?

@wei840222
Copy link
Contributor Author

wei840222 commented Jun 24, 2021

This proposal is good, for the default impl in Context.Deadline() Context.Done() Context.Err() seems no actual behavior.
This seems to increase the ease of use.
Maybe I can add this in another pr ?

@thinkerou
Copy link
Member

This proposal is good, for the default impl in Context.Deadline() Context.Done() Context.Err() seems no actual behavior.
This seems to increase the ease of use.
Maybe I can add this in another pr ?

please, thanks!

@pavelpatrin
Copy link

Still not in any release? Maybe release as a 1.7.8?

@jerome-laforge
Copy link
Contributor

jerome-laforge commented May 31, 2022

@wei840222 @appleboy @thinkerou This may be break change, I always use c.Request.Context(), It can be cancel or timeout when I use middleware. We should Done() and Deadline(),Err() API fallback c.Request.Done(), c.Request.Deadline(),c.Request.Err() too ?

As said by @thinkgos, this change makes a break change #3166

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.

10 participants