Skip to content

Swap http.Handler with Gin Gonic - #3850

Merged
mdomasevicius merged 8 commits into
masterfrom
cleanup
Aug 25, 2021
Merged

Swap http.Handler with Gin Gonic#3850
mdomasevicius merged 8 commits into
masterfrom
cleanup

Conversation

@mdomasevicius

Copy link
Copy Markdown
Collaborator

Since we use Gin in all other projects

@codecov-commenter

codecov-commenter commented Aug 24, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3850 (e09a126) into master (8bcc571) will decrease coverage by 0.16%.
The diff coverage is 49.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3850      +/-   ##
==========================================
- Coverage   43.46%   43.29%   -0.17%     
==========================================
  Files         326      324       -2     
  Lines       16808    16949     +141     
==========================================
+ Hits         7305     7338      +33     
- Misses       8757     8859     +102     
- Partials      746      752       +6     
Impacted Files Coverage Δ
tequilapi/endpoints/auth.go 0.00% <0.00%> (ø)
tequilapi/endpoints/config.go 0.00% <0.00%> (ø)
tequilapi/endpoints/entertainment.go 0.00% <0.00%> (ø)
tequilapi/endpoints/feedback.go 0.00% <0.00%> (ø)
tequilapi/endpoints/mmn.go 0.00% <0.00%> (ø)
tequilapi/endpoints/pprof.go 0.00% <0.00%> (ø)
tequilapi/endpoints/session_connectivity.go 0.00% <0.00%> (ø)
tequilapi/endpoints/sse.go 0.00% <0.00%> (ø)
tequilapi/endpoints/terms.go 0.00% <0.00%> (ø)
ui/reverse_proxy.go 20.83% <0.00%> (ø)
... and 26 more

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 8bcc571...e09a126. Read the comment docs.

@tomasmik tomasmik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we add a flag or something to control gin mode? By default both should be loaded with release and have the possibility to switch to debug if needed. It now prints logs of debug logs which are probably not needed.

Comment thread cmd/bootstrap.go Outdated
func (api *authenticationAPI) Login(httpRes http.ResponseWriter, httpReq *http.Request, _ httprouter.Params) {
func (api *authenticationAPI) Login(c *gin.Context) {
httpReq := c.Request
httpRes := c.Writer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well this seems a bit backwards to me. From a quick search it seems that most of the SendError or WriteAsJSON are only used in tequila and they all write JSON. So in theory functions in tequilapi/utils/utils.go could just accept interface:

type JSONErrorWriter interface {
	JSON(code int, body interface{})
}

And you could just pass c into those funcs. I guess this could also be improvement num 2 🤷🏻

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Like it says in the commit description: Quick & Dirty, I would like to throw away those utils and maybe swap them with a dependency to standardize this across all projects that use gin. So I would wish to leave this for another iteration :) this PR is large as it is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure no problem, just pointing an issue to open after this is merged for the future.

Comment thread tequilapi/endpoints/connection.go Outdated
Comment thread tequilapi/endpoints/connection_test.go Outdated
Comment thread tequilapi/endpoints/docs_test.go Outdated
Comment thread tequilapi/endpoints/identities.go Outdated
Comment thread tequilapi/endpoints/transactor.go Outdated
Comment thread tequilapi/http_api_server.go
@mdomasevicius
mdomasevicius requested a review from tomasmik August 24, 2021 11:25
Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
Signed-off-by: Mantas Domaševičius <mantas@mysterium.network>
@mdomasevicius
mdomasevicius merged commit b2ee821 into master Aug 25, 2021
@mdomasevicius
mdomasevicius deleted the cleanup branch August 25, 2021 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants