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.

Sign up for GitHub

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

Jump to bottom

Http refactor #3842

Merged

ncw merged 7 commits into rclone:master from innovate-invent:http_refactor

Apr 28, 2021

Contributor

innovate-invent commented Jan 2, 2020 •

edited

Loading

What is the purpose of this change?

Refactor http server code to abstract away the http server from the http services.

Was the change discussed in an issue or in the forum before?

https://forum.rclone.org/t/http-upload-contribution/13271/34

Checklist

I have read the contribution guidelines.
I have added tests for all changes in this PR if appropriate.
I have added documentation for the changes if appropriate.
All commit messages are in house style.
I'm done, this Pull Request is ready for review :-)

golangcibot reviewed

View reviewed changes

lib/http/http.go Outdated Show resolved Hide resolved

lib/http/auth/basic.go Show resolved Hide resolved

cmd/serve/http/http.go Outdated Show resolved Hide resolved

lib/http/http.go Show resolved Hide resolved

lib/http/http.go Show resolved Hide resolved

lib/http/auth/basic.go Show resolved Hide resolved

lib/http/auth/basic.go Show resolved Hide resolved

lib/http/auth/basic.go Show resolved Hide resolved

lib/http/auth/basic.go Show resolved Hide resolved

lib/http/auth/basic.go Outdated Show resolved Hide resolved

innovate-invent force-pushed the http_refactor branch 2 times, most recently from 052935d to e0eb25d Compare

January 2, 2020 01:04

golangcibot reviewed

View reviewed changes

lib/http/auth/basic.go Outdated Show resolved Hide resolved

lib/http/auth/basic.go Outdated Show resolved Hide resolved

lib/http/auth/basic.go Outdated Show resolved Hide resolved

lib/http/auth/basic.go Outdated Show resolved Hide resolved

innovate-invent force-pushed the http_refactor branch from e0eb25d to a4ae970 Compare

January 2, 2020 01:16

golangcibot reviewed

View reviewed changes

lib/http/http.go Outdated Show resolved Hide resolved

lib/http/http.go Outdated Show resolved Hide resolved

lib/http/http.go Outdated Show resolved Hide resolved

innovate-invent force-pushed the http_refactor branch 2 times, most recently from 4ffba70 to a434215 Compare

January 2, 2020 01:21

golangcibot reviewed

View reviewed changes

lib/http/http.go Outdated Show resolved Hide resolved

lib/http/http.go Outdated Show resolved Hide resolved

lib/http/http.go Outdated Show resolved Hide resolved

innovate-invent force-pushed the http_refactor branch 3 times, most recently from 7ad626a to fc2f498 Compare

January 2, 2020 01:37

golangcibot reviewed

View reviewed changes

lib/http/auth/auth.go Outdated Show resolved Hide resolved

innovate-invent force-pushed the http_refactor branch 2 times, most recently from 16f6a8d to 8eac907 Compare

January 2, 2020 02:08

Contributor Author

innovate-invent commented Jan 2, 2020

The failure is possibly due to golangci/golangci-lint#138

innovate-invent force-pushed the http_refactor branch from 8eac907 to 2db05a3 Compare

January 4, 2020 07:26

innovate-invent closed this

innovate-invent reopened this

innovate-invent force-pushed the http_refactor branch 2 times, most recently from 600d1e0 to 8f4e538 Compare

June 25, 2020 17:23

ivandeex added breaking change waiting for reply... labels

ivandeex added maintenance Serving refactoring needs rebase and removed maintenance labels

Member

ivandeex commented Dec 3, 2020

@innovate-invent
May I help you with rebasing this on master?

Contributor Author

innovate-invent commented Dec 3, 2020

@ivandeex absolutely, thanks

Member

ncw commented Apr 19, 2021

Is this getting squash merged or should I rewrite the commit history?

If you can re-write the commit history into a small number of logical changes then it will make it much easier to review and it will look good in the commit log. I think this change (which is quite big) should probably be more than one commit. Thanks

innovate-invent force-pushed the http_refactor branch from 5197fb4 to 469c78a Compare

April 20, 2021 05:17

Contributor Author

innovate-invent commented Apr 21, 2021

@ncw @ivandeex should be ready to merge

Member

ncw commented Apr 22, 2021

Can you rebase it on master to get rid of the merge commit?

I need to give this another review - will try to find time this week.

Contributor Author

innovate-invent commented Apr 22, 2021

I'll rebase, but master is a moving target

innovate-invent force-pushed the http_refactor branch 4 times, most recently from 9c92342 to 5dec9a1 Compare

April 23, 2021 21:06

Contributor Author

innovate-invent commented Apr 23, 2021

@ncw Just pushed a minor tweak and now it is saying you need to approve workflows?

ncw reviewed

View reviewed changes

Member

ncw left a comment

I think this is looking very good :-)

It is a nice re-organization of the http code.

I put a small number of minor comments inline for you to take a look at.

lib/http/http.go Outdated Show resolved Hide resolved

lib/http/http.go Outdated Show resolved Hide resolved

lib/http/http.go Outdated Show resolved Hide resolved

lib/http/http.go Show resolved Hide resolved

cmd/serve/httplib/httplib.go Show resolved Hide resolved

innovate-invent force-pushed the http_refactor branch from 5dec9a1 to 2e86505 Compare

April 26, 2021 16:48

innovate-invent requested a review from ncw

April 26, 2021 18:02

innovate-invent force-pushed the http_refactor branch from 2e86505 to 0e477e4 Compare

April 26, 2021 23:59

ncw approved these changes

View reviewed changes

Member

ncw left a comment

I'm happy with these changes now so I'll merge them now :-)

Can you fix the conflict first though? Rebase on master should do it.

Thanks

Nolan Woods added 7 commits

April 28, 2021 10:33


          feat: Add lib/http

95d00a9

lib/http provides an abstraction for a central http server that services can bind routes to


          lib/http: Add authentication middleware with basic auth implementation

2f29bbc


          lib/http: Move HTTP object serialization logic to lib/http

2848b68


          httplib: Add --template config and flags to serve/data

b8307c2


          httplib: Move httplib/serve/data to ../serve/http/data

8230f84


          httplib: Deprecate package

a2341f5


          http: Replace httplib with lib/http

987806b

innovate-invent force-pushed the http_refactor branch from 0e477e4 to 987806b Compare

April 28, 2021 17:33

Contributor Author

innovate-invent commented Apr 28, 2021 •

edited

Loading

Rebase completed, I'll open PRs refactoring the other services ASAP.

Member

ncw commented Apr 28, 2021

I'll merge that now.

Thank you for your great work and sorry it has taken so long to merge.

Look forward to the next commits in the series :-)

Thanks

ncw merged commit 5d2e327 into rclone:master

innovate-invent deleted the http_refactor branch

April 28, 2021 21:57

This was referenced May 2, 2021

restic: refactor to use lib/http #5294

Closed

lib/http: add default 404 handler #5295

Merged

http: clean up Bind to better use middleware #5296

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Serving