pkg/server: Trim public API to return *http.Server #246
Conversation
openshift-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wking The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
wking
force-pushed
the
server-api-reroll
branch
from
85dfda9
to
acf0ae5
Compare
wking
force-pushed
the
server-api-reroll
branch
from
acf0ae5
to
a15a8b1
Compare
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
IMO i liked the interface and i |
wking
force-pushed
the
server-api-reroll
branch
from
a15a8b1
to
8eef742
Compare
Do you have suggestions for testing the complete muxer with the approach master's currently taking? Currently it's tucked in there with the code launching listen-and-serve goroutines. And, if we're looking for smaller chunks of 8eef742816f1 where we agree, do you like |
wking
force-pushed
the
server-api-reroll
branch
2 times, most recently
from
a9d032b
to
5f0a6a7
Compare
|
unit: Dunno what that's about. /retest |
The previous API required consumers to construct a handler and pass
that handler back into a separate function to create a server based on
the handler. With this commit, I've rerolled the API to return the
handler directly, and the new public API is just NewBootstrapServer
and NewClusterServer. To serve both HTTP and HTTPS from the returned
server, I'm making shallow copies and altering Addr to set the
per-protocol ports in those copies.
I've also made some "server" -> "config" renames in the types that
have getConfig methods. And APIHandler is now configHandler. That
helps consolidate the /config/* handling under a consistent name, so
we have:
* An http.Server that is the server (e.g. as returned by
NewClusterServer).
* That server's handler is a muxer, and one of the muxer handlers is
the configHandler for /config/*.
* The configHandler structure holds a getConfig function, and we have
bootstrapConfig and clusterConfig which can supply getConfig. Using
a function type for this avoids the need for an interface and mock
getConfig type.
The defaultHandler gives us our usual empty 404s for requests that
don't match our other handlers. Without it, we'd have returned Go's
default:
404 page not found
body, and had unit-test failures like:
--- FAIL: TestHandler (0.00s)
--- FAIL: TestHandler/GET_/does-not-exist (0.00s)
api_test.go:93: response body length 19 does not match Content-Length -1
wking
force-pushed
the
server-api-reroll
branch
from
5f0a6a7
to
6cfa51c
Compare
|
Testing looks happy now 😄 |
openshift-ci-robot
added
the
needs-rebase
|
@wking: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
With #267 landed and #239 obsolete, I don't have anything consuming this anymore. I'll leave further API decisions in this package up to @abhinavdahiya and the other maintainers ;). /close |
|
@wking: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…managed Bug 1813175: abort metrics in general if removed/unmanaged
Builds on #240; review that first.
The previous API required consumers to construct a handler and pass that handler back into a separate function to create a server based on the handler. With this commit, I've rerolled the API to return the handler directly, and the new public API is just
NewBootstrapServerandNewClusterServer. To serve both HTTP and HTTPS from the returned server, I'm making shallow copies and alteringAddrto set the per-protocol ports in those copies.I've also made some
server->configrenames in the types that havegetConfigmethods. AndAPIHandleris nowconfigHandler. That helps consolidate the/config/*handling under a consistent name, so we have:*http.Serverthat is the server (e.g. as returned byNewClusterServer).configHandlerfor/config/*.configHandlerstructure holds agetConfigfunction, and we havebootstrapConfigandclusterConfigwhich can supplygetConfig. Using a function type for this avoids the need for an interface and mockgetConfigtype.