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

refactor: make route code much more siplified #988

Merged
merged 1 commit into from
Mar 29, 2018

Conversation

allencloud
Copy link
Collaborator

Signed-off-by: Allen Sun allensun.shl@alibaba-inc.com

Ⅰ. Describe what this PR did

I think without this PR, code adding routes to pouch server is very messy.
This PR makes the routed adding much more simplified.

Typically, I change r.Path(versionMatcher + "/containers/{name:.*}").Methods(http.MethodDelete).Handler(s.filter(s.removeContainers)) into addRoute(r, http.MethodDelete, "/containers/{name:.*}", s.removeContainers).

And add a function which is named addRoute.

In code change func (s *Server) filter(handler handler) http.HandlerFunc {, I think there is no need to add (s *Server).

Ⅱ. Does this pull request fix one issue?

none

Ⅲ. Describe how you did it

none

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

@faycheng
Copy link
Contributor

hi, @allencloud
It's a great job,and there are two tips.
1, /containers/{id:.*} seems a little odd,could we change it to /containers/:id for making it more readable?
2, maybe the arguments of addRoute could be less, could we change it to addGetRoute(r, "/images/search", s.searchImages)?

Looking forward to your reply.

Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
@allencloud
Copy link
Collaborator Author

@faycheng

Can we ensure that /containers/{id:.*} and /containers/:id have no difference?

In addition, I think addRoute and addGetRoute(r, "/images/search", s.searchImages) are almost the same. The reason I keep my own is that I usually use the way POST /a/b/c Handler This will makes me feel reasonable with complete information there.

@codecov-io
Copy link

Codecov Report

Merging #988 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #988   +/-   ##
=======================================
  Coverage   13.72%   13.72%           
=======================================
  Files         124      124           
  Lines        8367     8367           
=======================================
  Hits         1148     1148           
  Misses       7120     7120           
  Partials       99       99

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 1d7d1ce...ad5cd31. Read the comment docs.

@HusterWan
Copy link
Contributor

LGTM

@HusterWan HusterWan merged commit 7fde09d into AliyunContainerService:master Mar 29, 2018
@allencloud allencloud deleted the refactor-routes branch March 29, 2018 08:56
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.

5 participants