Move HTTP/handler utils to a separate package #1311
Conversation
Moving forward, they will be needed to implement Helm 3 support (see vmware-tanzu#1309), and they were in an internal package, so they couldn't be imported outside the `cmd/tiller-proxy` directory. Because they were all pure (or, in the case of ServeHTTP, at least very simple), I felt confident moving them to another file and reusing them.
| @@ -49,7 +50,7 @@ func TestErrorCodeWithDefault(t *testing.T) { | |||
| {fmt.Errorf("This is an unexpected error"), http.StatusUnprocessableEntity, http.StatusUnprocessableEntity}, | |||
| } | |||
| for _, s := range tests { | |||
| code := errorCodeWithDefault(s.err, s.defaultCode) | |||
| code := handlerutil.ErrorCodeWithDefault(s.err, s.defaultCode) | |||
There was a problem hiding this comment.
since this test is just testing ErrorCodeWithDefault, it's better to move this test to kg/handlerutil/handlerutil_test.go
There was a problem hiding this comment.
My only question is whether we need to replicate the WithParams etc. in new code. I mentioned in this comment that I don't see why we have them in tiller proxy (that is, over using standard handler func signature (w http.ResponseWriter, r *httpRequest) and a simple function to extract args), but i don't care either way. So if you think it's useful to keep using them, great.
You might be right, @absoludity. I'll see what I can do as soon as I have a usable internet connection again. Can't |
|
Merging this as it is, if you want to add something else, you can add it in a new PR :) |
|
OK, @andresmgot! I tried removing the EDIT: Found the reason in the tests. |
|
By the way, @absoludity, I think I can confirm (after a lengthy and painful debugging session) that the |
Thanks for digging there. I'm very interested to know why the request would not contain the same data when passed to |
As suggested by @absoludity: vmware-tanzu#1311 (review)
I may have been wrong in stating that it "doesn't contain any useful data to extract anymore" above. It seems at least as likely that it never did to begin with. You can see what I tried on the |
Description of the change
This PR moves a few types and (mostly pure) functions to a separate package.
Benefits
The code in question becomes more reusable, which will help with implementing Helm 3 support down the road (see #1309).
Possible drawbacks
Currently, this will expose some entities that weren't exposed before (
Params,WithParams,WithoutParams,ErrorCodeandErrorCodeWithDefault). If this is undesired, I'd be happy to create the new package at e.g.internal/handlerutilorcmd/internal/handlerutilinstead ofpkg/handlerutil. (AFAIK,pkg/internal/handlerutilwon't work because the moved entities need to be imported incmd.)